| Message ID | 20201001235337.83948-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: only query VNC when enabled | expand |
On Thu, Oct 01, 2020 at 07:53:37PM -0400, Jason Andryuk wrote: > QEMU without VNC support (configure --disable-vnc) will return an error > when VNC is queried over QMP since it does not recognize the QMP > command. This will cause libxl to fail starting the domain even if VNC > is not enabled. Therefore only query QEMU for VNC support when using > VNC, so a VNC-less QEMU will function in this configuration. > > 'goto out' jumps to the call to device_model_postconfig_done(), the > final callback after the chain of vnc queries. This bypasses all the > QMP VNC queries. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- > tools/libs/light/libxl_dm.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > index a944181781..d1ff35dda3 100644 > --- a/tools/libs/light/libxl_dm.c > +++ b/tools/libs/light/libxl_dm.c > @@ -3140,6 +3140,7 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > { > EGC_GC; > libxl__dm_spawn_state *dmss = CONTAINER_OF(qmp, *dmss, qmp); > + const libxl_vnc_info *vnc = libxl__dm_vnc(dmss->guest_config); > const libxl__json_object *item = NULL; > const libxl__json_object *o = NULL; > int i = 0; > @@ -3197,6 +3198,9 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > if (rc) goto out; > } > > + if (!vnc) > + goto out; > + I would rather this check be done in device_model_postconfig_vnc. Does the following work for you? diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c index a944181781bb..c5db755a65d7 100644 --- a/tools/libs/light/libxl_dm.c +++ b/tools/libs/light/libxl_dm.c @@ -3222,6 +3222,8 @@ static void device_model_postconfig_vnc(libxl__egc *egc, if (rc) goto out; + if (!vnc) goto out; + /* * query-vnc response: * { 'enabled': 'bool', '*host': 'str', '*service': 'str' } @@ -3255,7 +3257,8 @@ static void device_model_postconfig_vnc(libxl__egc *egc, if (rc) goto out; } - if (vnc && vnc->passwd && vnc->passwd[0]) { + assert(vnc); + if (vnc->passwd && vnc->passwd[0]) { qmp->callback = device_model_postconfig_vnc_passwd; libxl__qmp_param_add_string(gc, &args, "password", vnc->passwd); rc = libxl__ev_qmp_send(egc, qmp, "change-vnc-password", args);
On Wed, Oct 7, 2020 at 6:50 AM Wei Liu <wl@xen.org> wrote: > > On Thu, Oct 01, 2020 at 07:53:37PM -0400, Jason Andryuk wrote: > > QEMU without VNC support (configure --disable-vnc) will return an error > > when VNC is queried over QMP since it does not recognize the QMP > > command. This will cause libxl to fail starting the domain even if VNC > > is not enabled. Therefore only query QEMU for VNC support when using > > VNC, so a VNC-less QEMU will function in this configuration. > > > > 'goto out' jumps to the call to device_model_postconfig_done(), the > > final callback after the chain of vnc queries. This bypasses all the > > QMP VNC queries. > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > --- > > tools/libs/light/libxl_dm.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > > index a944181781..d1ff35dda3 100644 > > --- a/tools/libs/light/libxl_dm.c > > +++ b/tools/libs/light/libxl_dm.c > > @@ -3140,6 +3140,7 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > > { > > EGC_GC; > > libxl__dm_spawn_state *dmss = CONTAINER_OF(qmp, *dmss, qmp); > > + const libxl_vnc_info *vnc = libxl__dm_vnc(dmss->guest_config); > > const libxl__json_object *item = NULL; > > const libxl__json_object *o = NULL; > > int i = 0; > > @@ -3197,6 +3198,9 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > > if (rc) goto out; > > } > > > > + if (!vnc) > > + goto out; > > + > > I would rather this check be done in device_model_postconfig_vnc. > > Does the following work for you? I like your version, but it doesn't work: libxl: debug: libxl_qmp.c:1883:libxl__ev_qmp_send: Domain 1: ev 0x55aa58417d88, cmd 'query-vnc' libxl: error: libxl_qmp.c:1836:qmp_ev_parse_error_messages: Domain 1:The command query-vnc has not been found libxl: error: libxl_dm.c:3321:device_model_postconfig_done: Domain 1:Post DM startup configs failed, rc=-29 When QEMU has vnc disabled, it doesn't recognize query-vnc. I looked at modifying qemu to support query-vnc even with --disable-vnc, but it was messy to untangle the QMP definitions. Since we are telling libxl not to use VNC, it makes sense not to query about it. Regards, Jason > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > index a944181781bb..c5db755a65d7 100644 > --- a/tools/libs/light/libxl_dm.c > +++ b/tools/libs/light/libxl_dm.c > @@ -3222,6 +3222,8 @@ static void device_model_postconfig_vnc(libxl__egc *egc, > > if (rc) goto out; > > + if (!vnc) goto out; > + > /* > * query-vnc response: > * { 'enabled': 'bool', '*host': 'str', '*service': 'str' } > @@ -3255,7 +3257,8 @@ static void device_model_postconfig_vnc(libxl__egc *egc, > if (rc) goto out; > } > > - if (vnc && vnc->passwd && vnc->passwd[0]) { > + assert(vnc); > + if (vnc->passwd && vnc->passwd[0]) { > qmp->callback = device_model_postconfig_vnc_passwd; > libxl__qmp_param_add_string(gc, &args, "password", vnc->passwd); > rc = libxl__ev_qmp_send(egc, qmp, "change-vnc-password", args); >
On Thu, Oct 8, 2020 at 9:31 AM Jason Andryuk <jandryuk@gmail.com> wrote: > > On Wed, Oct 7, 2020 at 6:50 AM Wei Liu <wl@xen.org> wrote: > > > > On Thu, Oct 01, 2020 at 07:53:37PM -0400, Jason Andryuk wrote: > > > QEMU without VNC support (configure --disable-vnc) will return an error > > > when VNC is queried over QMP since it does not recognize the QMP > > > command. This will cause libxl to fail starting the domain even if VNC > > > is not enabled. Therefore only query QEMU for VNC support when using > > > VNC, so a VNC-less QEMU will function in this configuration. > > > > > > 'goto out' jumps to the call to device_model_postconfig_done(), the > > > final callback after the chain of vnc queries. This bypasses all the > > > QMP VNC queries. > > > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > > --- > > > tools/libs/light/libxl_dm.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > > > index a944181781..d1ff35dda3 100644 > > > --- a/tools/libs/light/libxl_dm.c > > > +++ b/tools/libs/light/libxl_dm.c > > > @@ -3140,6 +3140,7 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > > > { > > > EGC_GC; > > > libxl__dm_spawn_state *dmss = CONTAINER_OF(qmp, *dmss, qmp); > > > + const libxl_vnc_info *vnc = libxl__dm_vnc(dmss->guest_config); > > > const libxl__json_object *item = NULL; > > > const libxl__json_object *o = NULL; > > > int i = 0; > > > @@ -3197,6 +3198,9 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > > > if (rc) goto out; > > > } > > > > > > + if (!vnc) > > > + goto out; > > > + > > > > I would rather this check be done in device_model_postconfig_vnc. > > > > Does the following work for you? > > I like your version, but it doesn't work: > libxl: debug: libxl_qmp.c:1883:libxl__ev_qmp_send: Domain 1: ev > 0x55aa58417d88, cmd 'query-vnc' > libxl: error: libxl_qmp.c:1836:qmp_ev_parse_error_messages: Domain > 1:The command query-vnc has not been found > libxl: error: libxl_dm.c:3321:device_model_postconfig_done: Domain > 1:Post DM startup configs failed, rc=-29 > > When QEMU has vnc disabled, it doesn't recognize query-vnc. I looked > at modifying qemu to support query-vnc even with --disable-vnc, but it > was messy to untangle the QMP definitions. Since we are telling libxl > not to use VNC, it makes sense not to query about it. Oh, I should add that QEMU needs a small patch to allow -vnc none in ui/vnc-stub.c when vnc is disabled. This is because libxl always passes -vnc none to ensure a default vnc is not created. Regards, Jason > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > > index a944181781bb..c5db755a65d7 100644 > > --- a/tools/libs/light/libxl_dm.c > > +++ b/tools/libs/light/libxl_dm.c > > @@ -3222,6 +3222,8 @@ static void device_model_postconfig_vnc(libxl__egc *egc, > > > > if (rc) goto out; > > > > + if (!vnc) goto out; > > + > > /* > > * query-vnc response: > > * { 'enabled': 'bool', '*host': 'str', '*service': 'str' } > > @@ -3255,7 +3257,8 @@ static void device_model_postconfig_vnc(libxl__egc *egc, > > if (rc) goto out; > > } > > > > - if (vnc && vnc->passwd && vnc->passwd[0]) { > > + assert(vnc); > > + if (vnc->passwd && vnc->passwd[0]) { > > qmp->callback = device_model_postconfig_vnc_passwd; > > libxl__qmp_param_add_string(gc, &args, "password", vnc->passwd); > > rc = libxl__ev_qmp_send(egc, qmp, "change-vnc-password", args); > >
On Thu, Oct 08, 2020 at 09:31:15AM -0400, Jason Andryuk wrote: > On Wed, Oct 7, 2020 at 6:50 AM Wei Liu <wl@xen.org> wrote: > > > > On Thu, Oct 01, 2020 at 07:53:37PM -0400, Jason Andryuk wrote: > > > QEMU without VNC support (configure --disable-vnc) will return an error > > > when VNC is queried over QMP since it does not recognize the QMP > > > command. This will cause libxl to fail starting the domain even if VNC > > > is not enabled. Therefore only query QEMU for VNC support when using > > > VNC, so a VNC-less QEMU will function in this configuration. > > > > > > 'goto out' jumps to the call to device_model_postconfig_done(), the > > > final callback after the chain of vnc queries. This bypasses all the > > > QMP VNC queries. > > > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > > --- > > > tools/libs/light/libxl_dm.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c > > > index a944181781..d1ff35dda3 100644 > > > --- a/tools/libs/light/libxl_dm.c > > > +++ b/tools/libs/light/libxl_dm.c > > > @@ -3140,6 +3140,7 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > > > { > > > EGC_GC; > > > libxl__dm_spawn_state *dmss = CONTAINER_OF(qmp, *dmss, qmp); > > > + const libxl_vnc_info *vnc = libxl__dm_vnc(dmss->guest_config); > > > const libxl__json_object *item = NULL; > > > const libxl__json_object *o = NULL; > > > int i = 0; > > > @@ -3197,6 +3198,9 @@ static void device_model_postconfig_chardev(libxl__egc *egc, > > > if (rc) goto out; > > > } > > > > > > + if (!vnc) > > > + goto out; > > > + > > > > I would rather this check be done in device_model_postconfig_vnc. > > > > Does the following work for you? > > I like your version, but it doesn't work: > libxl: debug: libxl_qmp.c:1883:libxl__ev_qmp_send: Domain 1: ev > 0x55aa58417d88, cmd 'query-vnc' > libxl: error: libxl_qmp.c:1836:qmp_ev_parse_error_messages: Domain > 1:The command query-vnc has not been found > libxl: error: libxl_dm.c:3321:device_model_postconfig_done: Domain > 1:Post DM startup configs failed, rc=-29 > > When QEMU has vnc disabled, it doesn't recognize query-vnc. I looked > at modifying qemu to support query-vnc even with --disable-vnc, but it > was messy to untangle the QMP definitions. Since we are telling libxl > not to use VNC, it makes sense not to query about it. Ah, my bad. I misread the code. By the time libxl enters device_model_postconfig_vnc the query-vnc command will have already been issued. So your patch is fine. Acked-by: Wei Liu <wl@xen.org>
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c index a944181781..d1ff35dda3 100644 --- a/tools/libs/light/libxl_dm.c +++ b/tools/libs/light/libxl_dm.c @@ -3140,6 +3140,7 @@ static void device_model_postconfig_chardev(libxl__egc *egc, { EGC_GC; libxl__dm_spawn_state *dmss = CONTAINER_OF(qmp, *dmss, qmp); + const libxl_vnc_info *vnc = libxl__dm_vnc(dmss->guest_config); const libxl__json_object *item = NULL; const libxl__json_object *o = NULL; int i = 0; @@ -3197,6 +3198,9 @@ static void device_model_postconfig_chardev(libxl__egc *egc, if (rc) goto out; } + if (!vnc) + goto out; + qmp->callback = device_model_postconfig_vnc; rc = libxl__ev_qmp_send(egc, qmp, "query-vnc", NULL); if (rc) goto out;
QEMU without VNC support (configure --disable-vnc) will return an error when VNC is queried over QMP since it does not recognize the QMP command. This will cause libxl to fail starting the domain even if VNC is not enabled. Therefore only query QEMU for VNC support when using VNC, so a VNC-less QEMU will function in this configuration. 'goto out' jumps to the call to device_model_postconfig_done(), the final callback after the chain of vnc queries. This bypasses all the QMP VNC queries. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- tools/libs/light/libxl_dm.c | 4 ++++ 1 file changed, 4 insertions(+)