[Xen-devel] [PATCH v1 0/2] Remove backend xen store entry on domain destroy

[Xen-devel] [PATCH v1 0/2] Remove backend xen store entry on domain destroy

[Oleksandr Grytsov]

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

We have added VKBD device with user space PV backend. But libxl can't
differentiate domain kind for this device. As result, it performs QEMU procedure
for adding and removing VKB device with user space backend. To fix this issue,
new device kind VINPUT is introduced. It is used as backend kind in case of
user space VKBD backend. 

Another issue addresses in this patch series is error timeout on guest domain
destroy in case using user space PV backends.

We have a driver domain with user space backends (VDISPL, VSND, VKBD). When a
guest domain destroyed, we see following error:

    "timed out while waiting for ... to be removed"

xl expects that PV device backend entries is removed. xl devd removes these
entries for specific devices only: VBD, VIF, QDISK and then deletes the guest
domain from the list. The fix is to delete guest domain only after all devices
are removed and performs generic device remove procedure for following device:
VINPUT, VDISPL, VSND.

Oleksandr Grytsov (2):
  libxl: introduce new backend type VINPUT
  libxl: add removing XS backend path for PV devices on domain destroy

 tools/libxl/libxl_device.c           | 14 +++++---------
 tools/libxl/libxl_types_internal.idl |  1 +
 tools/libxl/libxl_vkb.c              | 29 ++++++++++++++++++----------
 3 files changed, 25 insertions(+), 19 deletions(-)

-- 
2.17.1


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

[Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Oleksandr Grytsov]

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

There are two kind of VKBD devices: with QEMU backend and user space
backend. In current implementation they can't be distinguished as both use
VKBD backend type. As result, user space KBD backend is started and
stopped as QEMU backend. This commit adds new device kind VINPUT to be
used as backend type for user space KBD backend.

Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
---
 tools/libxl/libxl_types_internal.idl |  1 +
 tools/libxl/libxl_vkb.c              | 29 ++++++++++++++++++----------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index cb85c3b37f..3593e21dbb 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -31,6 +31,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (13, "VUART"),
     (14, "PVCALLS"),
     (15, "VSND"),
+    (16, "VINPUT"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
diff --git a/tools/libxl/libxl_vkb.c b/tools/libxl/libxl_vkb.c
index 26376a7eef..4c44a813c1 100644
--- a/tools/libxl/libxl_vkb.c
+++ b/tools/libxl/libxl_vkb.c
@@ -38,9 +38,6 @@ static int libxl__set_xenstore_vkb(libxl__gc *gc, uint32_t domid,
                                    flexarray_t *back, flexarray_t *front,
                                    flexarray_t *ro_front)
 {
-    flexarray_append_pair(back, "backend-type",
-                          (char *)libxl_vkb_backend_to_string(vkb->backend_type));
-
     if (vkb->unique_id) {
         flexarray_append_pair(back, XENKBD_FIELD_UNIQUE_ID, vkb->unique_id);
     }
@@ -93,7 +90,8 @@ static int libxl__vkb_from_xenstore(libxl__gc *gc, const char *libxl_path,
                                     libxl_devid devid,
                                     libxl_device_vkb *vkb)
 {
-    const char *be_path, *be_type, *fe_path, *tmp;
+    const char *be_path, *fe_path, *tmp;
+    libxl__device dev;
     int rc;
 
     vkb->devid = devid;
@@ -111,13 +109,11 @@ static int libxl__vkb_from_xenstore(libxl__gc *gc, const char *libxl_path,
     rc = libxl__backendpath_parse_domid(gc, be_path, &vkb->backend_domid);
     if (rc) goto out;
 
-    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
-                                  GCSPRINTF("%s/backend-type", be_path),
-                                  &be_type);
+    rc = libxl__parse_backend_path(gc, be_path, &dev);
     if (rc) goto out;
 
-    rc = libxl_vkb_backend_from_string(be_type, &vkb->backend_type);
-    if (rc) goto out;
+    vkb->backend_type = dev.backend_kind == LIBXL__DEVICE_KIND_VINPUT ?
+                                            LIBXL_VKB_BACKEND_LINUX : LIBXL_VKB_BACKEND_QEMU;
 
     vkb->unique_id = xs_read(CTX->xsh, XBT_NULL, GCSPRINTF("%s/"XENKBD_FIELD_UNIQUE_ID, be_path), NULL);
 
@@ -218,6 +214,20 @@ out:
     return rc;
 }
 
+static int libxl__device_from_vkb(libxl__gc *gc, uint32_t domid,
+                                  libxl_device_vkb *type, libxl__device *device)
+{
+    device->backend_devid   = type->devid;
+    device->backend_domid   = type->backend_domid;
+    device->backend_kind    = type->backend_type == LIBXL_VKB_BACKEND_LINUX ?
+                              LIBXL__DEVICE_KIND_VINPUT : LIBXL__DEVICE_KIND_VKBD;
+    device->devid           = type->devid;
+    device->domid           = domid;
+    device->kind            = LIBXL__DEVICE_KIND_VKBD;
+
+    return 0;
+}
+
 int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb,
                          const libxl_asyncop_how *ao_how)
 {
@@ -318,7 +328,6 @@ out:
      return rc;
 }
 
-static LIBXL_DEFINE_DEVICE_FROM_TYPE(vkb)
 static LIBXL_DEFINE_UPDATE_DEVID(vkb)
 
 #define libxl__add_vkbs NULL
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Ian Jackson]

Oleksandr Grytsov writes ("[PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> There are two kind of VKBD devices: with QEMU backend and user space
> backend. In current implementation they can't be distinguished as both use
> VKBD backend type. As result, user space KBD backend is started and
> stopped as QEMU backend. This commit adds new device kind VINPUT to be
> used as backend type for user space KBD backend.

I find this confusing.  I'm not sure this change is right.  But I'm
afraid the original patches in this area passed me by so I don't know
much about it.

I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
which introduced what you are calling "user space" backends.  It
appears that the vkb backend type enum was introduced there
specifically to distinguish between these two situations.  For reasons

Am I wrong ?  If not, why is this not working or not suitable ?

I also don't understand why the "user space" kbd backend seems to be
"linux" in the enum.  Where is the implementation of this user space
backend ?  Is it be controlled entirely through xenstore ?

Ian.

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Oleksandr Grytsov]

On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Oleksandr Grytsov writes ("[PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >
> > There are two kind of VKBD devices: with QEMU backend and user space
> > backend. In current implementation they can't be distinguished as both use
> > VKBD backend type. As result, user space KBD backend is started and
> > stopped as QEMU backend. This commit adds new device kind VINPUT to be
> > used as backend type for user space KBD backend.
>
> I find this confusing.  I'm not sure this change is right.  But I'm
> afraid the original patches in this area passed me by so I don't know
> much about it.
>
> I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> which introduced what you are calling "user space" backends.  It
> appears that the vkb backend type enum was introduced there
> specifically to distinguish between these two situations.  For reasons
>
> Am I wrong ?  If not, why is this not working or not suitable ?

You are right. It is not working in some cases due to QEMU_BACKEND macro.
QEMU_BACKEND treats both backends as QEMU. As result xl performs device
hotplug on add/remove device. Which is not expected in case "user
space" backend.

In this patch I propose solution similar to VUSB device. Where VUSB
used for frontend
and depends on backend (kernel or QEMU) either VUSB or QUSB used for backend
device type e.g. use different backend device type for different
backends. It would be
more clear if, for example, QEMU backend has QKBD name and all other
VKBD. But it
would require changes on QEMU side too. That's why I've chosen VINPUT name.

Introducing new backend device type for VKBD also allows to have both backends
(QEMU and non QEMU) run in same domain. Not sure if it is useful
scenario but with
this proposal it is possible from technical point of view.

>
> I also don't understand why the "user space" kbd backend seems to be
> "linux" in the enum.

I agree this is not so good name. But I don't know how to call
backends which are not running
inside QEMU in general.

> Where is the implementation of this user space
> backend ?

We have extended kbd interface (kbdif.h) to support multi-touch events
as well. And we have
implemented own kbd backend https://github.com/xen-troops/displ_be/
It is integrated with display backend as both use wayland API.

> Is it be controlled entirely through xenstore ?

Yes it is controlled entirely through xenstore: lib xl creates
frontend/backend entries with
initial states and configuration.

> Ian.

-- 
Best Regards,
Oleksandr Grytsov.

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Ian Jackson]

Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> > which introduced what you are calling "user space" backends.  It
> > appears that the vkb backend type enum was introduced there
> > specifically to distinguish between these two situations.  For reasons
> >
> > Am I wrong ?  If not, why is this not working or not suitable ?
> 
> You are right. It is not working in some cases due to QEMU_BACKEND macro.
> QEMU_BACKEND treats both backends as QEMU. As result xl performs device
> hotplug on add/remove device. Which is not expected in case "user
> space" backend.

Then perhaps this macro needs to be adjusted or called only
conditionally or something ?

> In this patch I propose solution similar to VUSB device. Where VUSB
> used for frontend and depends on backend (kernel or QEMU) either
> VUSB or QUSB used for backend device type e.g. use different backend
> device type for different backends.

I confess I don't quite follow all the VUSB stuff but I don't think it
is necessarily a good model.

> Introducing new backend device type for VKBD also allows to have
> both backends (QEMU and non QEMU) run in same domain. Not sure if it
> is useful scenario but with this proposal it is possible from
> technical point of view.

I don't understand why this is not possible simply by having a
different backend type.

> > I also don't understand why the "user space" kbd backend seems to be
> > "linux" in the enum.
> 
> I agree this is not so good name. But I don't know how to call
> backends which are not running
> inside QEMU in general.

I think this would be useable on freebsd ?  "linux" definitely seems
wrong.  I see it hasn't been in a release so it is not too late to
rename it, subject to discussion with Juergen as RM.

> > Where is the implementation of this user space
> > backend ?
> 
> We have extended kbd interface (kbdif.h) to support multi-touch events
> as well. And we have
> implemented own kbd backend https://github.com/xen-troops/displ_be/
> It is integrated with display backend as both use wayland API.

Great.

> > Is it be controlled entirely through xenstore ?
> 
> Yes it is controlled entirely through xenstore: lib xl creates
> frontend/backend entries with
> initial states and configuration.

And your display backend in "troops" (is that the name of your
project) checks whether the backend type is set to "linux", so that it
knows to ignore ones that say "qemu" ?

Maybe "linux" should be "troops"...

Ian.

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Oleksandr Grytsov]

On Fri, Oct 11, 2019 at 8:04 PM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > > I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> > > which introduced what you are calling "user space" backends.  It
> > > appears that the vkb backend type enum was introduced there
> > > specifically to distinguish between these two situations.  For reasons
> > >
> > > Am I wrong ?  If not, why is this not working or not suitable ?
> >
> > You are right. It is not working in some cases due to QEMU_BACKEND macro.
> > QEMU_BACKEND treats both backends as QEMU. As result xl performs device
> > hotplug on add/remove device. Which is not expected in case "user
> > space" backend.
>
> Then perhaps this macro needs to be adjusted or called only
> conditionally or something ?

I had an idea to move this macro to libxl__device_type and let device
itself decides
if it is qemu backend. But in case of VKBD it will read XS to determine backend
type. I guess it is ok.

>
> > In this patch I propose solution similar to VUSB device. Where VUSB
> > used for frontend and depends on backend (kernel or QEMU) either
> > VUSB or QUSB used for backend device type e.g. use different backend
> > device type for different backends.
>
> I confess I don't quite follow all the VUSB stuff but I don't think it
> is necessarily a good model.

If you don't mind to move QEMU_BACKEND macrto to libxl__device_type then
no need to add new device type at all.

>
> > Introducing new backend device type for VKBD also allows to have
> > both backends (QEMU and non QEMU) run in same domain. Not sure if it
> > is useful scenario but with this proposal it is possible from
> > technical point of view.
>
> I don't understand why this is not possible simply by having a
> different backend type.
>
> > > I also don't understand why the "user space" kbd backend seems to be
> > > "linux" in the enum.
> >
> > I agree this is not so good name. But I don't know how to call
> > backends which are not running
> > inside QEMU in general.
>
> I think this would be useable on freebsd ?  "linux" definitely seems
> wrong.  I see it hasn't been in a release so it is not too late to
> rename it, subject to discussion with Juergen as RM.
>
> > > Where is the implementation of this user space
> > > backend ?
> >
> > We have extended kbd interface (kbdif.h) to support multi-touch events
> > as well. And we have
> > implemented own kbd backend https://github.com/xen-troops/displ_be/
> > It is integrated with display backend as both use wayland API.
>
> Great.
>
> > > Is it be controlled entirely through xenstore ?
> >
> > Yes it is controlled entirely through xenstore: lib xl creates
> > frontend/backend entries with
> > initial states and configuration.
>
> And your display backend in "troops" (is that the name of your
> project) checks whether the backend type is set to "linux", so that it
> knows to ignore ones that say "qemu" ?
>
> Maybe "linux" should be "troops"...
>

It doesn't look as generic solution. If some user implements own backend
it should add new entry into backend type enum.
What about to have just string value instead of enum? In case QEMU
we don't have such entry at all but in case custom backend the user
can't put any string value here to be recognized by his backend.

> Ian.

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Oleksandr Grytsov]

On Wed, Oct 16, 2019 at 4:26 PM Oleksandr Grytsov <al1img@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 8:04 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> >
> > Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > > On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > > > I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> > > > which introduced what you are calling "user space" backends.  It
> > > > appears that the vkb backend type enum was introduced there
> > > > specifically to distinguish between these two situations.  For reasons
> > > >
> > > > Am I wrong ?  If not, why is this not working or not suitable ?
> > >
> > > You are right. It is not working in some cases due to QEMU_BACKEND macro.
> > > QEMU_BACKEND treats both backends as QEMU. As result xl performs device
> > > hotplug on add/remove device. Which is not expected in case "user
> > > space" backend.
> >
> > Then perhaps this macro needs to be adjusted or called only
> > conditionally or something ?
>
> I had an idea to move this macro to libxl__device_type and let device
> itself decides
> if it is qemu backend. But in case of VKBD it will read XS to determine backend
> type. I guess it is ok.
>
> >
> > > In this patch I propose solution similar to VUSB device. Where VUSB
> > > used for frontend and depends on backend (kernel or QEMU) either
> > > VUSB or QUSB used for backend device type e.g. use different backend
> > > device type for different backends.
> >
> > I confess I don't quite follow all the VUSB stuff but I don't think it
> > is necessarily a good model.
>
> If you don't mind to move QEMU_BACKEND macrto to libxl__device_type then
> no need to add new device type at all.
>
> >
> > > Introducing new backend device type for VKBD also allows to have
> > > both backends (QEMU and non QEMU) run in same domain. Not sure if it
> > > is useful scenario but with this proposal it is possible from
> > > technical point of view.
> >
> > I don't understand why this is not possible simply by having a
> > different backend type.
> >
> > > > I also don't understand why the "user space" kbd backend seems to be
> > > > "linux" in the enum.
> > >
> > > I agree this is not so good name. But I don't know how to call
> > > backends which are not running
> > > inside QEMU in general.
> >
> > I think this would be useable on freebsd ?  "linux" definitely seems
> > wrong.  I see it hasn't been in a release so it is not too late to
> > rename it, subject to discussion with Juergen as RM.
> >
> > > > Where is the implementation of this user space
> > > > backend ?
> > >
> > > We have extended kbd interface (kbdif.h) to support multi-touch events
> > > as well. And we have
> > > implemented own kbd backend https://github.com/xen-troops/displ_be/
> > > It is integrated with display backend as both use wayland API.
> >
> > Great.
> >
> > > > Is it be controlled entirely through xenstore ?
> > >
> > > Yes it is controlled entirely through xenstore: lib xl creates
> > > frontend/backend entries with
> > > initial states and configuration.
> >
> > And your display backend in "troops" (is that the name of your
> > project) checks whether the backend type is set to "linux", so that it
> > knows to ignore ones that say "qemu" ?
> >
> > Maybe "linux" should be "troops"...
> >
>
> It doesn't look as generic solution. If some user implements own backend
> it should add new entry into backend type enum.
> What about to have just string value instead of enum? In case QEMU
> we don't have such entry at all but in case custom backend the user
> can't put any string value here to be recognized by his backend.
>
> > Ian.

ping

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Oleksandr Grytsov]

On Mon, Oct 28, 2019 at 4:06 PM Oleksandr Grytsov <al1img@gmail.com> wrote:
>
> On Wed, Oct 16, 2019 at 4:26 PM Oleksandr Grytsov <al1img@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 8:04 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > >
> > > Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > > > On Fri, Oct 11, 2019 at 5:58 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > > > > I think it was a48e00f14a2d "libxl: add backend type and id to vkb"
> > > > > which introduced what you are calling "user space" backends.  It
> > > > > appears that the vkb backend type enum was introduced there
> > > > > specifically to distinguish between these two situations.  For reasons
> > > > >
> > > > > Am I wrong ?  If not, why is this not working or not suitable ?
> > > >
> > > > You are right. It is not working in some cases due to QEMU_BACKEND macro.
> > > > QEMU_BACKEND treats both backends as QEMU. As result xl performs device
> > > > hotplug on add/remove device. Which is not expected in case "user
> > > > space" backend.
> > >
> > > Then perhaps this macro needs to be adjusted or called only
> > > conditionally or something ?
> >
> > I had an idea to move this macro to libxl__device_type and let device
> > itself decides
> > if it is qemu backend. But in case of VKBD it will read XS to determine backend
> > type. I guess it is ok.
> >
> > >
> > > > In this patch I propose solution similar to VUSB device. Where VUSB
> > > > used for frontend and depends on backend (kernel or QEMU) either
> > > > VUSB or QUSB used for backend device type e.g. use different backend
> > > > device type for different backends.
> > >
> > > I confess I don't quite follow all the VUSB stuff but I don't think it
> > > is necessarily a good model.
> >
> > If you don't mind to move QEMU_BACKEND macrto to libxl__device_type then
> > no need to add new device type at all.
> >
> > >
> > > > Introducing new backend device type for VKBD also allows to have
> > > > both backends (QEMU and non QEMU) run in same domain. Not sure if it
> > > > is useful scenario but with this proposal it is possible from
> > > > technical point of view.
> > >
> > > I don't understand why this is not possible simply by having a
> > > different backend type.
> > >
> > > > > I also don't understand why the "user space" kbd backend seems to be
> > > > > "linux" in the enum.
> > > >
> > > > I agree this is not so good name. But I don't know how to call
> > > > backends which are not running
> > > > inside QEMU in general.
> > >
> > > I think this would be useable on freebsd ?  "linux" definitely seems
> > > wrong.  I see it hasn't been in a release so it is not too late to
> > > rename it, subject to discussion with Juergen as RM.
> > >
> > > > > Where is the implementation of this user space
> > > > > backend ?
> > > >
> > > > We have extended kbd interface (kbdif.h) to support multi-touch events
> > > > as well. And we have
> > > > implemented own kbd backend https://github.com/xen-troops/displ_be/
> > > > It is integrated with display backend as both use wayland API.
> > >
> > > Great.
> > >
> > > > > Is it be controlled entirely through xenstore ?
> > > >
> > > > Yes it is controlled entirely through xenstore: lib xl creates
> > > > frontend/backend entries with
> > > > initial states and configuration.
> > >
> > > And your display backend in "troops" (is that the name of your
> > > project) checks whether the backend type is set to "linux", so that it
> > > knows to ignore ones that say "qemu" ?
> > >
> > > Maybe "linux" should be "troops"...
> > >
> >
> > It doesn't look as generic solution. If some user implements own backend
> > it should add new entry into backend type enum.
> > What about to have just string value instead of enum? In case QEMU
> > we don't have such entry at all but in case custom backend the user
> > can't put any string value here to be recognized by his backend.
> >
> > > Ian.
>
> ping

ping

Ian,

I'm waiting for your comments about following questions:

1. Move QEMU_BACKEND macro to libxl__device_type structure as function
    and let the device to decide it has QEMU backend:

struct libxl__device_type {
    ...
    device_qemu_backend_fn_t qemu_backend
}

In this case, introducing new device type for VKBD is not needed. The VKBD
device will check backend type XS entry to define which backend is running.

2. Use string type for VKBD backend_type field instead of enum. It will be
empty for QEMU and generic for "user space" backends.

Thanks.

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Ian Jackson]

Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> 1. Move QEMU_BACKEND macro to libxl__device_type structure as function
>     and let the device to decide it has QEMU backend:
> 
> struct libxl__device_type {
>     ...
>     device_qemu_backend_fn_t qemu_backend
> }
> 
> In this case, introducing new device type for VKBD is not needed. The VKBD
> device will check backend type XS entry to define which backend is running.

Sorry for the delay replying.  In your earlier mails I had trouble
figuring out what you meant but this little vignette makes it clear to
me.

I think the problem you are trying to solve is this: in your case
QEMU_BACKEND needs to depend on the visible vkb_backend field, but the
device->backend_kind is set unconditionally to just VKB ?

Could you solve this problem by inventing a new backend_kind, and
writing your own function libxl__device_from_vkb, and putting
*different* values into backend_kind ?  I think that is what
backend_kind is for.  See for example various console functions and
also libxl__device_from_disk.

> 2. Use string type for VKBD backend_type field instead of enum. It will be
> empty for QEMU and generic for "user space" backends.

This seems worse.

> On Mon, Oct 28, 2019 at 4:06 PM Oleksandr Grytsov <al1img@gmail.com> wrote:
> > On Wed, Oct 16, 2019 at 4:26 PM Oleksandr Grytsov <al1img@gmail.com> wrote:
> > > [Ian:]
> > > > [Oleksandr:]
> > > > > [Ian:]
> > > > > > I also don't understand why the "user space" kbd backend seems to be
> > > > > > "linux" in the enum.
> > > > >
> > > > > I agree this is not so good name. But I don't know how to call
> > > > > backends which are not running
> > > > > inside QEMU in general.
> > > >
> > > > I think this would be useable on freebsd ?  "linux" definitely seems
> > > > wrong.  I see it hasn't been in a release so it is not too late to
> > > > rename it, subject to discussion with Juergen as RM.
...
> > > > Maybe "linux" should be "troops"...
> > >
> > > It doesn't look as generic solution. If some user implements own backend
> > > it should add new entry into backend type enum.

Would you be prepared to change it to *something* else ?

AFAICT from the code it just uses what would the `usual' xenstore pv
control plane path for a device called "vkb" ?

So maybe we could call it "pv" ?  Is there a protocol doc I should be
looking at that defines this vkb interface ?

Sorry still to be so confused.

Regards,
Ian.

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Oleksandr Grytsov]

On Fri, Nov 15, 2019 at 9:43 PM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > 1. Move QEMU_BACKEND macro to libxl__device_type structure as function
> >     and let the device to decide it has QEMU backend:
> >
> > struct libxl__device_type {
> >     ...
> >     device_qemu_backend_fn_t qemu_backend
> > }
> >
> > In this case, introducing new device type for VKBD is not needed. The VKBD
> > device will check backend type XS entry to define which backend is running.
>
> Sorry for the delay replying.  In your earlier mails I had trouble
> figuring out what you meant but this little vignette makes it clear to
> me.
>
> I think the problem you are trying to solve is this: in your case
> QEMU_BACKEND needs to depend on the visible vkb_backend field, but the
> device->backend_kind is set unconditionally to just VKB ?

Exactly.

>
> Could you solve this problem by inventing a new backend_kind, and
> writing your own function libxl__device_from_vkb, and putting
> *different* values into backend_kind ?  I think that is what
> backend_kind is for.  See for example various console functions and
> also libxl__device_from_disk.
>

This what was done in this patch. VINPUT backend type was introduced.
Probably the name should be changed but have no idea which backend
kind is more suitable for this purpose.

Also bakcend-type xenstore entry was removed as redundant in this case.
As the PV backend expects device kind VINPUT.

> > 2. Use string type for VKBD backend_type field instead of enum. It will be
> > empty for QEMU and generic for "user space" backends.
>
> This seems worse.
>
> > On Mon, Oct 28, 2019 at 4:06 PM Oleksandr Grytsov <al1img@gmail.com> wrote:
> > > On Wed, Oct 16, 2019 at 4:26 PM Oleksandr Grytsov <al1img@gmail.com> wrote:
> > > > [Ian:]
> > > > > [Oleksandr:]
> > > > > > [Ian:]
> > > > > > > I also don't understand why the "user space" kbd backend seems to be
> > > > > > > "linux" in the enum.
> > > > > >
> > > > > > I agree this is not so good name. But I don't know how to call
> > > > > > backends which are not running
> > > > > > inside QEMU in general.
> > > > >
> > > > > I think this would be useable on freebsd ?  "linux" definitely seems
> > > > > wrong.  I see it hasn't been in a release so it is not too late to
> > > > > rename it, subject to discussion with Juergen as RM.
> ...
> > > > > Maybe "linux" should be "troops"...
> > > >
> > > > It doesn't look as generic solution. If some user implements own backend
> > > > it should add new entry into backend type enum.
>
> Would you be prepared to change it to *something* else ?
>
> AFAICT from the code it just uses what would the `usual' xenstore pv
> control plane path for a device called "vkb" ?
>

I guess yes.

> So maybe we could call it "pv" ?

Do you mean LIBXL_VKB_BACKEND_PV?

> Is there a protocol doc I should be
> looking at that defines this vkb interface ?
>

This PV backend utilizes the protocol described in kbdif.h

> Sorry still to be so confused.
>
> Regards,
> Ian.



-- 
Best Regards,
Oleksandr Grytsov.

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

Re: [Xen-devel] [PATCH v1 1/2] libxl: introduce new backend type VINPUT

[Ian Jackson]

Oleksandr Grytsov writes ("Re: [PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> On Fri, Nov 15, 2019 at 9:43 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > Sorry for the delay replying.  In your earlier mails I had trouble
> > figuring out what you meant but this little vignette makes it clear to
> > me.
> >
> > I think the problem you are trying to solve is this: in your case
> > QEMU_BACKEND needs to depend on the visible vkb_backend field, but the
> > device->backend_kind is set unconditionally to just VKB ?
> 
> Exactly.

Thanks.

> > Could you solve this problem by inventing a new backend_kind, and
> > writing your own function libxl__device_from_vkb, and putting
> > *different* values into backend_kind ?  I think that is what
> > backend_kind is for.  See for example various console functions and
> > also libxl__device_from_disk.
> 
> This what was done in this patch. VINPUT backend type was introduced.

We have come full circle.  But on the way you have managed to get into
my thick head what is going on here.  Well done and thank you.

I will go back and do a more code review of the original patch.

> Probably the name should be changed but have no idea which backend
> kind is more suitable for this purpose.

I am happy this this name.  It is not in the public API so if it turns
out to be wrong we can change it.

> > AFAICT from the code it just uses what would the `usual' xenstore pv
> > control plane path for a device called "vkb" ?
> 
> I guess yes.
> 
> > So maybe we could call it "pv" ?
> 
> Do you mean LIBXL_VKB_BACKEND_PV?

I think so.  What do you think ?  I am just trying to get rid of the
string `linux' when it's not Linux specific.  I quickly scanned
kbdif.h and it looks very like a Xen PV protocol :-).  So "pv" would
sound good to me and better than "Linux", unless someone else has an
opinion.

CCing various maintainers who might have an opinion.

Ian.

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

Re: [Xen-devel] [PATCH for-4.13 v1 1/2] libxl: introduce new backend type VINPUT

[Ian Jackson]

Oleksandr Grytsov writes ("[PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> There are two kind of VKBD devices: with QEMU backend and user space
> backend. In current implementation they can't be distinguished as both use
> VKBD backend type. As result, user space KBD backend is started and
> stopped as QEMU backend. This commit adds new device kind VINPUT to be
> used as backend type for user space KBD backend.

Thank you for this patch and thank you for the explanations.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I wasn't able to find a posting of this patch before the last posting
date for 4.13 of the 13th of September.  Have I missed it ?  We might
be able to justify a freeze exception on the grounds that this change
affects only vkb users but it would be a matter for the RM (CC'd).


I would like to change the "linux" to "pv" or something else, for
4.13, at least.

Ian.

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

Re: [Xen-devel] [PATCH for-4.13 v1 1/2] libxl: introduce new backend type VINPUT

[Oleksandr Grytsov]

On Mon, Nov 18, 2019 at 7:55 PM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Oleksandr Grytsov writes ("[PATCH v1 1/2] libxl: introduce new backend type VINPUT"):
> > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >
> > There are two kind of VKBD devices: with QEMU backend and user space
> > backend. In current implementation they can't be distinguished as both use
> > VKBD backend type. As result, user space KBD backend is started and
> > stopped as QEMU backend. This commit adds new device kind VINPUT to be
> > used as backend type for user space KBD backend.
>
> Thank you for this patch and thank you for the explanations.
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> I wasn't able to find a posting of this patch before the last posting
> date for 4.13 of the 13th of September.  Have I missed it ?  We might
> be able to justify a freeze exception on the grounds that this change
> affects only vkb users but it would be a matter for the RM (CC'd).
>

This commit was submitted with patcheset [1].
Earlier I've submitted the patch to solve the issue with patchest [2].
But that patchet was totally wrong.

>
> I would like to change the "linux" to "pv" or something else, for
> 4.13, at least.

I will submit V2 with renaming and comments addressed for second commit [3]
of the patchset.

>
> Ian.

[1] https://marc.info/?l=xen-devel&m=157054390006691&w=2
[2] https://marc.info/?l=xen-devel&m=151326089604524&w=2
[3] https://marc.info/?l=xen-devel&m=157054391506708&w=2


--
Best Regards,
Oleksandr Grytsov.

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

Re: [Xen-devel] [PATCH for-4.13 v1 1/2] libxl: introduce new backend type VINPUT

[Ian Jackson]

Oleksandr Grytsov writes ("Re: [PATCH for-4.13 v1 1/2] libxl: introduce new backend type VINPUT"):
> I will submit V2 with renaming and comments addressed for second commit [3]
> of the patchset.

Thanks.  Juergen tells this is OK in principle but me he wants to take
only critical fixes after the next RC.  So please be quick if you can
:-).

Thanks,
Ian.

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

[Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy

[Oleksandr Grytsov]

From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Currently backend path is remove for specific devices: VBD, VIF, QDISK.
This commit adds removing backend path for: VDISPL, VSND, VINPUT.

Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
---
 tools/libxl/libxl_device.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 1402b61a81..8ce70661e5 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1477,7 +1477,7 @@ typedef struct libxl__ddomain_device {
  */
 typedef struct libxl__ddomain_guest {
     uint32_t domid;
-    int num_vifs, num_vbds, num_qdisks;
+    int num_qdisks;
     LIBXL_SLIST_HEAD(, struct libxl__ddomain_device) devices;
     LIBXL_SLIST_ENTRY(struct libxl__ddomain_guest) next;
 } libxl__ddomain_guest;
@@ -1530,8 +1530,7 @@ static void check_and_maybe_remove_guest(libxl__gc *gc,
 {
     assert(ddomain);
 
-    if (dguest != NULL &&
-        dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks == 0) {
+    if (dguest != NULL && LIBXL_SLIST_FIRST(&dguest->devices) == NULL) {
         LIBXL_SLIST_REMOVE(&ddomain->guests, dguest, libxl__ddomain_guest,
                            next);
         LOGD(DEBUG, dguest->domid, "Removed domain from the list of active guests");
@@ -1573,9 +1572,6 @@ static int add_device(libxl__egc *egc, libxl__ao *ao,
     switch(dev->backend_kind) {
     case LIBXL__DEVICE_KIND_VBD:
     case LIBXL__DEVICE_KIND_VIF:
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds++;
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs++;
-
         GCNEW(aodev);
         libxl__prepare_ao_device(ao, aodev);
         /*
@@ -1619,11 +1615,11 @@ static int remove_device(libxl__egc *egc, libxl__ao *ao,
     int rc = 0;
 
     switch(ddev->dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VDISPL:
+    case LIBXL__DEVICE_KIND_VSND:
+    case LIBXL__DEVICE_KIND_VINPUT:
     case LIBXL__DEVICE_KIND_VBD:
     case LIBXL__DEVICE_KIND_VIF:
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
-        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
-
         GCNEW(aodev);
         libxl__prepare_ao_device(ao, aodev);
         /*
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy

[Ian Jackson]

Oleksandr Grytsov writes ("[PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Currently backend path is remove for specific devices: VBD, VIF, QDISK.
> This commit adds removing backend path for: VDISPL, VSND, VINPUT.

Thanks for this and your previous patch.

This one looks to me like it is probably correct.  But I have not been
able to understand why this code was limited to vbds and vifs before
so I am hesitant.

Searching the git history, I think this has been like this since
a0eaa86e7537
 "libxl: add device backend listener in order to launch backends"
and subsequent commits have just reorganised things but not
fundamentally changed them.  I've CC'd Roger who wrote this code.

I think:

>      switch(ddev->dev->backend_kind) {
> +    case LIBXL__DEVICE_KIND_VDISPL:
> +    case LIBXL__DEVICE_KIND_VSND:
> +    case LIBXL__DEVICE_KIND_VINPUT:
>      case LIBXL__DEVICE_KIND_VBD:
>      case LIBXL__DEVICE_KIND_VIF:

If we do want this to handle *all* kinds of device, maybe it should
have a fallback that aborts, or something ?  (I don't think it is
easy to make it a compile error to forget to add an entry here but if
we could do that it would probably be best.)

All of that assuming that the basic idea is right, which I would like
Roger's opinion about.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy

[Roger Pau Monné]

On Fri, Oct 11, 2019 at 04:07:11PM +0100, Ian Jackson wrote:
> Oleksandr Grytsov writes ("[PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > 
> > Currently backend path is remove for specific devices: VBD, VIF, QDISK.
> > This commit adds removing backend path for: VDISPL, VSND, VINPUT.
> 
> Thanks for this and your previous patch.
> 
> This one looks to me like it is probably correct.  But I have not been
> able to understand why this code was limited to vbds and vifs before
> so I am hesitant.
> 
> Searching the git history, I think this has been like this since
> a0eaa86e7537
>  "libxl: add device backend listener in order to launch backends"
> and subsequent commits have just reorganised things but not
> fundamentally changed them.  I've CC'd Roger who wrote this code.

When this code was added (devd) those where the only backends handled
by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
the issue is that when support for those was added, it wasn't properly
wired into devd.

> I think:
> 
> >      switch(ddev->dev->backend_kind) {
> > +    case LIBXL__DEVICE_KIND_VDISPL:
> > +    case LIBXL__DEVICE_KIND_VSND:
> > +    case LIBXL__DEVICE_KIND_VINPUT:
> >      case LIBXL__DEVICE_KIND_VBD:
> >      case LIBXL__DEVICE_KIND_VIF:
> 
> If we do want this to handle *all* kinds of device, maybe it should
> have a fallback that aborts, or something ?  (I don't think it is
> easy to make it a compile error to forget to add an entry here but if
> we could do that it would probably be best.)

Maybe we could have some generic handling for everything != qdisk?

IIRC qdisk needs special handling so that devd can launch and destroy
a QEMU instance when required, but other backends that run in the
kernel don't need such handling and could maybe share the same generic
code path.

> All of that assuming that the basic idea is right, which I would like
> Roger's opinion about.

Looking at the patch itself, you also seem to be doing some changes
related to num_vbds and num_vifs, but those are not mentioned in the
commit message, is that a stray change?

I'm not saying it's wrong, it's just that it feels like it belongs in
a different patch maybe.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy

[Ian Jackson]

Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> When this code was added (devd) those where the only backends handled
> by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> the issue is that when support for those was added, it wasn't properly
> wired into devd.
> 
> > I think:
> > 
> > >      switch(ddev->dev->backend_kind) {
> > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > +    case LIBXL__DEVICE_KIND_VSND:
> > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > >      case LIBXL__DEVICE_KIND_VBD:
> > >      case LIBXL__DEVICE_KIND_VIF:
> > 
> > If we do want this to handle *all* kinds of device, maybe it should
> > have a fallback that aborts, or something ?  (I don't think it is
> > easy to make it a compile error to forget to add an entry here but if
> > we could do that it would probably be best.)
> 
> Maybe we could have some generic handling for everything != qdisk?

So make that the "default:" ?  That would be fine by me.

> IIRC qdisk needs special handling so that devd can launch and destroy
> a QEMU instance when required, but other backends that run in the
> kernel don't need such handling and could maybe share the same generic
> code path.

Right.

> > All of that assuming that the basic idea is right, which I would like
> > Roger's opinion about.
> 
> Looking at the patch itself, you also seem to be doing some changes
> related to num_vbds and num_vifs, but those are not mentioned in the
> commit message, is that a stray change?

No, I don't think so.  Those variables just tell us when the thing is
torn down but Oleksandr's code is able to use the devices slist itself
for that.  Please do check to see if you agree.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy

[Roger Pau Monné]

On Fri, Oct 11, 2019 at 04:55:32PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > When this code was added (devd) those where the only backends handled
> > by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> > the issue is that when support for those was added, it wasn't properly
> > wired into devd.
> > 
> > > I think:
> > > 
> > > >      switch(ddev->dev->backend_kind) {
> > > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > > +    case LIBXL__DEVICE_KIND_VSND:
> > > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > > >      case LIBXL__DEVICE_KIND_VBD:
> > > >      case LIBXL__DEVICE_KIND_VIF:
> > > 
> > > If we do want this to handle *all* kinds of device, maybe it should
> > > have a fallback that aborts, or something ?  (I don't think it is
> > > easy to make it a compile error to forget to add an entry here but if
> > > we could do that it would probably be best.)
> > 
> > Maybe we could have some generic handling for everything != qdisk?
> 
> So make that the "default:" ?  That would be fine by me.

If possible yes, that would be my preference, and would prevent having
to add new device types to this switch unless special handling is
required.

> 
> > IIRC qdisk needs special handling so that devd can launch and destroy
> > a QEMU instance when required, but other backends that run in the
> > kernel don't need such handling and could maybe share the same generic
> > code path.
> 
> Right.
> 
> > > All of that assuming that the basic idea is right, which I would like
> > > Roger's opinion about.
> > 
> > Looking at the patch itself, you also seem to be doing some changes
> > related to num_vbds and num_vifs, but those are not mentioned in the
> > commit message, is that a stray change?
> 
> No, I don't think so.  Those variables just tell us when the thing is
> torn down but Oleksandr's code is able to use the devices slist itself
> for that.  Please do check to see if you agree.

Yes, that's fine. I don't think those changes are wrong, I just think
that at least they should be mentioned in the commit message. Ie:
"while there remove the num_vifs and num_vbds since they are not
needed and the same can be achieved by checking that the device list
is empty." or some such.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy

[Oleksandr Grytsov]

On Tue, Oct 15, 2019 at 6:39 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Fri, Oct 11, 2019 at 04:55:32PM +0100, Ian Jackson wrote:
> > Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > > When this code was added (devd) those where the only backends handled
> > > by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> > > the issue is that when support for those was added, it wasn't properly
> > > wired into devd.
> > >
> > > > I think:
> > > >
> > > > >      switch(ddev->dev->backend_kind) {
> > > > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > > > +    case LIBXL__DEVICE_KIND_VSND:
> > > > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > > > >      case LIBXL__DEVICE_KIND_VBD:
> > > > >      case LIBXL__DEVICE_KIND_VIF:
> > > >
> > > > If we do want this to handle *all* kinds of device, maybe it should
> > > > have a fallback that aborts, or something ?  (I don't think it is
> > > > easy to make it a compile error to forget to add an entry here but if
> > > > we could do that it would probably be best.)
> > >
> > > Maybe we could have some generic handling for everything != qdisk?
> >
> > So make that the "default:" ?  That would be fine by me.
>
> If possible yes, that would be my preference, and would prevent having
> to add new device types to this switch unless special handling is
> required.
>
> >
> > > IIRC qdisk needs special handling so that devd can launch and destroy
> > > a QEMU instance when required, but other backends that run in the
> > > kernel don't need such handling and could maybe share the same generic
> > > code path.
> >
> > Right.
> >
> > > > All of that assuming that the basic idea is right, which I would like
> > > > Roger's opinion about.
> > >
> > > Looking at the patch itself, you also seem to be doing some changes
> > > related to num_vbds and num_vifs, but those are not mentioned in the
> > > commit message, is that a stray change?
> >
> > No, I don't think so.  Those variables just tell us when the thing is
> > torn down but Oleksandr's code is able to use the devices slist itself
> > for that.  Please do check to see if you agree.
>
> Yes, that's fine. I don't think those changes are wrong, I just think
> that at least they should be mentioned in the commit message. Ie:
> "while there remove the num_vifs and num_vbds since they are not
> needed and the same can be achieved by checking that the device list
> is empty." or some such.
>
> Thanks, Roger.

Ian, Roger,

Thanks for reviewing and comments. I will update the patch with your
comments above.

-- 
Best Regards,
Oleksandr Grytsov.

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