[PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Marek Marczykowski-Górecki]

Use xen-blkback for 'vbd' disk types by default and do not setup vfb+vkb
when no access method was configured. Then check if qemu is really
needed.

The only not configurable thing forcing qemu running in dom0 after this
change are consoles used to save/restore. But even in that case, there
is much smaller part of qemu exposed.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_disk.c |  4 +++-
 tools/libxl/libxl_dm.c   | 52 ++++++++++++++++++++++++++++++++++--------------
 2 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 63de75c..7842d9b 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -56,10 +56,12 @@ static void disk_eject_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *w,
             "/local/domain/%d/backend/%" TOSTRING(BACKEND_STRING_SIZE)
            "[a-z]/%*d/%*d",
            &disk->backend_domid, backend_type);
-    if (!strcmp(backend_type, "tap") || !strcmp(backend_type, "vbd")) {
+    if (!strcmp(backend_type, "tap")) {
         disk->backend = LIBXL_DISK_BACKEND_TAP;
     } else if (!strcmp(backend_type, "qdisk")) {
         disk->backend = LIBXL_DISK_BACKEND_QDISK;
+    } else if (!strcmp(backend_type, "vbd")) {
+        disk->backend = LIBXL_DISK_BACKEND_PHY;
     } else {
         disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
     }
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 44ebd70..c9aefe15 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1868,13 +1868,17 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
     if (ret) goto out;
 
-    GCNEW(vfb);
-    GCNEW(vkb);
-    libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
-    dm_config->vfbs = vfb;
-    dm_config->num_vfbs = 1;
-    dm_config->vkbs = vkb;
-    dm_config->num_vkbs = 1;
+    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
+            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
+            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {
+        GCNEW(vfb);
+        GCNEW(vkb);
+        libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
+        dm_config->vfbs = vfb;
+        dm_config->num_vfbs = 1;
+        dm_config->vkbs = vkb;
+        dm_config->num_vkbs = 1;
+    }
 
     stubdom_state->pv_kernel.path
         = libxl__abs_path(gc, "ioemu-stubdom.gz", libxl__xenfirmwaredir_path());
@@ -1959,6 +1963,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
     libxl__domain_build_state *const d_state = sdss->dm.build_state;
     libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
     uint32_t dm_domid = sdss->pvqemu.guest_domid;
+    int need_qemu;
 
     if (ret) {
         LOGD(ERROR, guest_domid, "error connecting disk devices");
@@ -1975,12 +1980,16 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
         if (ret)
             goto out;
     }
-    ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
-    if (ret)
-        goto out;
-    ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
-    if (ret)
-        goto out;
+    if (dm_config->num_vfbs) {
+        ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
+        if (ret)
+            goto out;
+    }
+    if (dm_config->num_vkbs) {
+        ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
+        if (ret)
+            goto out;
+    }
 
     if (guest_config->b_info.u.hvm.serial)
         num_console++;
@@ -1988,7 +1997,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
     console = libxl__calloc(gc, num_console, sizeof(libxl__device_console));
 
     for (i = 0; i < num_console; i++) {
-        libxl__device device;
         console[i].devid = i;
         console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
         /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
@@ -2005,6 +2013,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
                 if (ret) goto out;
                 console[i].output = GCSPRINTF("file:%s", filename);
                 free(filename);
+                /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEMU if qemu
+                 * will be in use */
+                console[i].consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
                 break;
             case STUBDOM_CONSOLE_SAVE:
                 console[i].output = GCSPRINTF("file:%s",
@@ -2019,6 +2030,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
                 console[i].output = "pty";
                 break;
         }
+    }
+
+    need_qemu = libxl__need_xenpv_qemu(gc, dm_config);
+
+    for (i = 0; i < num_console; i++) {
+        libxl__device device;
         ret = libxl__device_console_add(gc, dm_domid, &console[i],
                         i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL,
                         &device);
@@ -2032,7 +2049,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
     sdss->pvqemu.build_state = &sdss->dm_state;
     sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb;
 
-    libxl__spawn_local_dm(egc, &sdss->pvqemu);
+    if (!need_qemu) {
+        /* If dom0 qemu not needed, do not launch it */
+        spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, 0);
+    } else {
+        libxl__spawn_local_dm(egc, &sdss->pvqemu);
+    }
 
     return;
 
-- 
2.7.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Wei Liu]

On Thu, Jul 27, 2017 at 12:44:25AM +0200, Marek Marczykowski-Górecki wrote:
> Use xen-blkback for 'vbd' disk types by default and do not setup vfb+vkb
> when no access method was configured. Then check if qemu is really
> needed.
> 
> The only not configurable thing forcing qemu running in dom0 after this
> change are consoles used to save/restore. But even in that case, there
> is much smaller part of qemu exposed.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl_disk.c |  4 +++-
>  tools/libxl/libxl_dm.c   | 52 ++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
> index 63de75c..7842d9b 100644
> --- a/tools/libxl/libxl_disk.c
> +++ b/tools/libxl/libxl_disk.c
> @@ -56,10 +56,12 @@ static void disk_eject_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *w,
>              "/local/domain/%d/backend/%" TOSTRING(BACKEND_STRING_SIZE)
>             "[a-z]/%*d/%*d",
>             &disk->backend_domid, backend_type);
> -    if (!strcmp(backend_type, "tap") || !strcmp(backend_type, "vbd")) {
> +    if (!strcmp(backend_type, "tap")) {
>          disk->backend = LIBXL_DISK_BACKEND_TAP;
>      } else if (!strcmp(backend_type, "qdisk")) {
>          disk->backend = LIBXL_DISK_BACKEND_QDISK;
> +    } else if (!strcmp(backend_type, "vbd")) {
> +        disk->backend = LIBXL_DISK_BACKEND_PHY;

This should be split into a separate patch.

>      } else {
>          disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
>      }
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 44ebd70..c9aefe15 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1868,13 +1868,17 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>      ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
>      if (ret) goto out;
>  
> -    GCNEW(vfb);
> -    GCNEW(vkb);
> -    libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> -    dm_config->vfbs = vfb;
> -    dm_config->num_vfbs = 1;
> -    dm_config->vkbs = vkb;
> -    dm_config->num_vkbs = 1;
> +    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
> +            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
> +            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {

Indentation.

> +        GCNEW(vfb);
> +        GCNEW(vkb);
> +        libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> +        dm_config->vfbs = vfb;
> +        dm_config->num_vfbs = 1;
> +        dm_config->vkbs = vkb;
> +        dm_config->num_vkbs = 1;
> +    }
>  
>      stubdom_state->pv_kernel.path
>          = libxl__abs_path(gc, "ioemu-stubdom.gz", libxl__xenfirmwaredir_path());
> @@ -1959,6 +1963,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>      libxl__domain_build_state *const d_state = sdss->dm.build_state;
>      libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
>      uint32_t dm_domid = sdss->pvqemu.guest_domid;
> +    int need_qemu;
>  
>      if (ret) {
>          LOGD(ERROR, guest_domid, "error connecting disk devices");
> @@ -1975,12 +1980,16 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>          if (ret)
>              goto out;
>      }
> -    ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
> -    if (ret)
> -        goto out;
> -    ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
> -    if (ret)
> -        goto out;
> +    if (dm_config->num_vfbs) {
> +        ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
> +        if (ret)
> +            goto out;
> +    }
> +    if (dm_config->num_vkbs) {
> +        ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
> +        if (ret)
> +            goto out;
> +    }
>  
>      if (guest_config->b_info.u.hvm.serial)
>          num_console++;
> @@ -1988,7 +1997,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>      console = libxl__calloc(gc, num_console, sizeof(libxl__device_console));
>  
>      for (i = 0; i < num_console; i++) {
> -        libxl__device device;
>          console[i].devid = i;
>          console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
>          /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
> @@ -2005,6 +2013,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>                  if (ret) goto out;
>                  console[i].output = GCSPRINTF("file:%s", filename);
>                  free(filename);
> +                /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEMU if qemu
> +                 * will be in use */
> +                console[i].consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;

Where is the code to change it back?

>                  break;
>              case STUBDOM_CONSOLE_SAVE:
>                  console[i].output = GCSPRINTF("file:%s",
> @@ -2019,6 +2030,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>                  console[i].output = "pty";
>                  break;
>          }
> +    }
> +
> +    need_qemu = libxl__need_xenpv_qemu(gc, dm_config);
> +
> +    for (i = 0; i < num_console; i++) {
> +        libxl__device device;
>          ret = libxl__device_console_add(gc, dm_domid, &console[i],
>                          i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL,
>                          &device);
> @@ -2032,7 +2049,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>      sdss->pvqemu.build_state = &sdss->dm_state;
>      sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb;
>  
> -    libxl__spawn_local_dm(egc, &sdss->pvqemu);
> +    if (!need_qemu) {
> +        /* If dom0 qemu not needed, do not launch it */
> +        spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, 0);
> +    } else {
> +        libxl__spawn_local_dm(egc, &sdss->pvqemu);
> +    }

Does this mean if the user doesn't configure certain devices he / she
won't be able to migrate a guest served by a qemu-stubdom? That's not
right IMO.

>  
>      return;
>  
> -- 
> 2.7.5
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 7240 bytes --]

On Fri, Jul 28, 2017 at 04:17:51PM +0100, Wei Liu wrote:
> On Thu, Jul 27, 2017 at 12:44:25AM +0200, Marek Marczykowski-Górecki wrote:
> > Use xen-blkback for 'vbd' disk types by default and do not setup vfb+vkb
> > when no access method was configured. Then check if qemu is really
> > needed.
> > 
> > The only not configurable thing forcing qemu running in dom0 after this
> > change are consoles used to save/restore. But even in that case, there
> > is much smaller part of qemu exposed.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >  tools/libxl/libxl_disk.c |  4 +++-
> >  tools/libxl/libxl_dm.c   | 52 ++++++++++++++++++++++++++++++++++--------------
> >  2 files changed, 40 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
> > index 63de75c..7842d9b 100644
> > --- a/tools/libxl/libxl_disk.c
> > +++ b/tools/libxl/libxl_disk.c
> > @@ -56,10 +56,12 @@ static void disk_eject_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *w,
> >              "/local/domain/%d/backend/%" TOSTRING(BACKEND_STRING_SIZE)
> >             "[a-z]/%*d/%*d",
> >             &disk->backend_domid, backend_type);
> > -    if (!strcmp(backend_type, "tap") || !strcmp(backend_type, "vbd")) {
> > +    if (!strcmp(backend_type, "tap")) {
> >          disk->backend = LIBXL_DISK_BACKEND_TAP;
> >      } else if (!strcmp(backend_type, "qdisk")) {
> >          disk->backend = LIBXL_DISK_BACKEND_QDISK;
> > +    } else if (!strcmp(backend_type, "vbd")) {
> > +        disk->backend = LIBXL_DISK_BACKEND_PHY;
> 
> This should be split into a separate patch.
> 
> >      } else {
> >          disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
> >      }
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 44ebd70..c9aefe15 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1868,13 +1868,17 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> >      ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
> >      if (ret) goto out;
> >  
> > -    GCNEW(vfb);
> > -    GCNEW(vkb);
> > -    libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> > -    dm_config->vfbs = vfb;
> > -    dm_config->num_vfbs = 1;
> > -    dm_config->vkbs = vkb;
> > -    dm_config->num_vkbs = 1;
> > +    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
> > +            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
> > +            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {
> 
> Indentation.
> 
> > +        GCNEW(vfb);
> > +        GCNEW(vkb);
> > +        libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> > +        dm_config->vfbs = vfb;
> > +        dm_config->num_vfbs = 1;
> > +        dm_config->vkbs = vkb;
> > +        dm_config->num_vkbs = 1;
> > +    }
> >  
> >      stubdom_state->pv_kernel.path
> >          = libxl__abs_path(gc, "ioemu-stubdom.gz", libxl__xenfirmwaredir_path());
> > @@ -1959,6 +1963,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >      libxl__domain_build_state *const d_state = sdss->dm.build_state;
> >      libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
> >      uint32_t dm_domid = sdss->pvqemu.guest_domid;
> > +    int need_qemu;
> >  
> >      if (ret) {
> >          LOGD(ERROR, guest_domid, "error connecting disk devices");
> > @@ -1975,12 +1980,16 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >          if (ret)
> >              goto out;
> >      }
> > -    ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
> > -    if (ret)
> > -        goto out;
> > -    ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
> > -    if (ret)
> > -        goto out;
> > +    if (dm_config->num_vfbs) {
> > +        ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
> > +        if (ret)
> > +            goto out;
> > +    }
> > +    if (dm_config->num_vkbs) {
> > +        ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
> > +        if (ret)
> > +            goto out;
> > +    }
> >  
> >      if (guest_config->b_info.u.hvm.serial)
> >          num_console++;
> > @@ -1988,7 +1997,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >      console = libxl__calloc(gc, num_console, sizeof(libxl__device_console));
> >  
> >      for (i = 0; i < num_console; i++) {
> > -        libxl__device device;
> >          console[i].devid = i;
> >          console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> >          /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
> > @@ -2005,6 +2013,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >                  if (ret) goto out;
> >                  console[i].output = GCSPRINTF("file:%s", filename);
> >                  free(filename);
> > +                /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEMU if qemu
> > +                 * will be in use */
> > +                console[i].consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
> 
> Where is the code to change it back?

Oh, libxl__need_xenpv_qemu did that previously, but I it doesn't have this
part anymore. Will add it here.

> >                  break;
> >              case STUBDOM_CONSOLE_SAVE:
> >                  console[i].output = GCSPRINTF("file:%s",
> > @@ -2019,6 +2030,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >                  console[i].output = "pty";
> >                  break;
> >          }
> > +    }
> > +
> > +    need_qemu = libxl__need_xenpv_qemu(gc, dm_config);
> > +
> > +    for (i = 0; i < num_console; i++) {
> > +        libxl__device device;
> >          ret = libxl__device_console_add(gc, dm_domid, &console[i],
> >                          i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL,
> >                          &device);
> > @@ -2032,7 +2049,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >      sdss->pvqemu.build_state = &sdss->dm_state;
> >      sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb;
> >  
> > -    libxl__spawn_local_dm(egc, &sdss->pvqemu);
> > +    if (!need_qemu) {
> > +        /* If dom0 qemu not needed, do not launch it */
> > +        spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, 0);
> > +    } else {
> > +        libxl__spawn_local_dm(egc, &sdss->pvqemu);
> > +    }
> 
> Does this mean if the user doesn't configure certain devices he / she
> won't be able to migrate a guest served by a qemu-stubdom? That's not
> right IMO.

This patch itself does not break it (yet). Such guest will have all
consoles needed for migration by default. This also means that need_qemu
will always be true in practice. I imagine the next step would be either
adding some option to disable those consoles (maybe name it
"break_migration"? ;) ). Or adding support for multiple consoles to
xenconsoled.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Wei Liu]

On Fri, Jul 28, 2017 at 05:34:42PM +0200, Marek Marczykowski-Górecki wrote:
[...]
> This patch itself does not break it (yet). Such guest will have all
> consoles needed for migration by default. This also means that need_qemu
> will always be true in practice. I imagine the next step would be either
> adding some option to disable those consoles (maybe name it
> "break_migration"? ;) ). Or adding support for multiple consoles to
> xenconsoled.
> 

The latter is preferably and a developer is already working on it (for a
different reason).

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1684 bytes --]

On Fri, Jul 28, 2017 at 04:17:51PM +0100, Wei Liu wrote:
> On Thu, Jul 27, 2017 at 12:44:25AM +0200, Marek Marczykowski-Górecki wrote:
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 44ebd70..c9aefe15 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1868,13 +1868,17 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> >      ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
> >      if (ret) goto out;
> >  
> > -    GCNEW(vfb);
> > -    GCNEW(vkb);
> > -    libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> > -    dm_config->vfbs = vfb;
> > -    dm_config->num_vfbs = 1;
> > -    dm_config->vkbs = vkb;
> > -    dm_config->num_vkbs = 1;
> > +    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
> > +            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
> > +            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {
> 
> Indentation.

Should it really be indented at the same level as the code inside this
block? Looks misleading (you need to look for ") {" to see where
condition ends).

> > +        GCNEW(vfb);
> > +        GCNEW(vkb);
> > +        libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> > +        dm_config->vfbs = vfb;
> > +        dm_config->num_vfbs = 1;
> > +        dm_config->vkbs = vkb;
> > +        dm_config->num_vkbs = 1;
> > +    }

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Wei Liu]

On Fri, Jul 28, 2017 at 06:05:13PM +0200, Marek Marczykowski-Górecki wrote:
> On Fri, Jul 28, 2017 at 04:17:51PM +0100, Wei Liu wrote:
> > On Thu, Jul 27, 2017 at 12:44:25AM +0200, Marek Marczykowski-Górecki wrote:
> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > > index 44ebd70..c9aefe15 100644
> > > --- a/tools/libxl/libxl_dm.c
> > > +++ b/tools/libxl/libxl_dm.c
> > > @@ -1868,13 +1868,17 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> > >      ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
> > >      if (ret) goto out;
> > >  
> > > -    GCNEW(vfb);
> > > -    GCNEW(vkb);
> > > -    libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> > > -    dm_config->vfbs = vfb;
> > > -    dm_config->num_vfbs = 1;
> > > -    dm_config->vkbs = vkb;
> > > -    dm_config->num_vkbs = 1;
> > > +    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
> > > +            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
> > > +            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {
> > 
> > Indentation.
> 
> Should it really be indented at the same level as the code inside this
> block? Looks misleading (you need to look for ") {" to see where
> condition ends).

That's how existing code in libxl is like, isn't it?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed"):
> On Fri, Jul 28, 2017 at 06:05:13PM +0200, Marek Marczykowski-Górecki wrote:
> > On Fri, Jul 28, 2017 at 04:17:51PM +0100, Wei Liu wrote:
> > > On Thu, Jul 27, 2017 at 12:44:25AM +0200, Marek Marczykowski-Górecki wrote:
> > > > +    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
> > > > +            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
> > > > +            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {
> > > 
> > > Indentation.
> > 
> > Should it really be indented at the same level as the code inside this
> > block? Looks misleading (you need to look for ") {" to see where
> > condition ends).
> 
> That's how existing code in libxl is like, isn't it?

Yes.  Sadly.

If you like we could have a bunfight about changing the indent level.
I like 2, personally :-P.

A compromise might be to move the || to the start of the next line, so

   if (cond1
       || cond2
       || cond3) {
       code1;
       code2;

I have done that sometimes in libxl and no-one has objected.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1510 bytes --]

On Fri, Jul 28, 2017 at 05:17:12PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed"):
> > On Fri, Jul 28, 2017 at 06:05:13PM +0200, Marek Marczykowski-Górecki wrote:
> > > On Fri, Jul 28, 2017 at 04:17:51PM +0100, Wei Liu wrote:
> > > > On Thu, Jul 27, 2017 at 12:44:25AM +0200, Marek Marczykowski-Górecki wrote:
> > > > > +    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
> > > > > +            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
> > > > > +            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {
> > > > 
> > > > Indentation.
> > > 
> > > Should it really be indented at the same level as the code inside this
> > > block? Looks misleading (you need to look for ") {" to see where
> > > condition ends).
> > 
> > That's how existing code in libxl is like, isn't it?
> 
> Yes.  Sadly.
> 
> If you like we could have a bunfight about changing the indent level.
> I like 2, personally :-P.
> 
> A compromise might be to move the || to the start of the next line, so
> 
>    if (cond1
>        || cond2
>        || cond3) {
>        code1;
>        code2;
> 
> I have done that sometimes in libxl and no-one has objected.

Ok, it would be much better, thanks.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel