diff mbox series

[v2,3/3] libxl: make default path to add/remove all PV devices

Message ID 20191121181300.6497-4-al1img@gmail.com (mailing list archive)
State New, archived
Headers show
Series Remove backend xen store entry on domain destroy | expand

Commit Message

Oleksandr Grytsov Nov. 21, 2019, 6:13 p.m. UTC 
From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Adding/removing device is handled for specific devices only: VBD, VIF,
QDISK. This commit adds default case to handle adding/removing for all PV
devices by default, except QDISK device, which requires special handling.
If any other device is required a special handling it should be done by
implementing separate case (similar to QDISK device). The default
behaviour for adding device is to wait when the backend goes to
XenbusStateInitWait and the default behaviour on removing device is to
start generic device remove procedure.

Also this commit fixes removing guest function: before the guest was
removed when all VIF and VBD devices are removed. The fix removes
guest when all created devices are removed. This is done by checking the
guest device list instead of checking num_vifs and num_vbds. num_vifs and
num_vbds variables are removed as redundant in this case.

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

Comments

Ian Jackson Nov. 22, 2019, 11:16 a.m. UTC | #1 
Oleksandr Grytsov writes ("[PATCH v2 3/3] libxl: make default path to add/remove all PV devices"):
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Adding/removing device is handled for specific devices only: VBD, VIF,
> QDISK. This commit adds default case to handle adding/removing for all PV
> devices by default, except QDISK device, which requires special handling.
> If any other device is required a special handling it should be done by
> implementing separate case (similar to QDISK device). The default
> behaviour for adding device is to wait when the backend goes to
> XenbusStateInitWait and the default behaviour on removing device is to
> start generic device remove procedure.
> 
> Also this commit fixes removing guest function: before the guest was
> removed when all VIF and VBD devices are removed. The fix removes
> guest when all created devices are removed. This is done by checking the
> guest device list instead of checking num_vifs and num_vbds. num_vifs and
> num_vbds variables are removed as redundant in this case.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Wei Liu Nov. 22, 2019, 12:01 p.m. UTC | #2 
On Thu, Nov 21, 2019 at 08:13:00PM +0200, Oleksandr Grytsov wrote:
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Adding/removing device is handled for specific devices only: VBD, VIF,
> QDISK. This commit adds default case to handle adding/removing for all PV
> devices by default, except QDISK device, which requires special handling.
> If any other device is required a special handling it should be done by
> implementing separate case (similar to QDISK device). The default
> behaviour for adding device is to wait when the backend goes to
> XenbusStateInitWait and the default behaviour on removing device is to
> start generic device remove procedure.
> 
> Also this commit fixes removing guest function: before the guest was
> removed when all VIF and VBD devices are removed. The fix removes
> guest when all created devices are removed. This is done by checking the
> guest device list instead of checking num_vifs and num_vbds. num_vifs and
> num_vbds variables are removed as redundant in this case.
> 
> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

I agree with this approach in general, but I haven't looked closely into
the implementation. FWIW:

Acked-by: Wei Liu <wl@xen.org>
diff mbox series

Patch

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 1402b61a81..9d05d2fd13 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");
@@ -1571,24 +1570,6 @@  static int add_device(libxl__egc *egc, libxl__ao *ao,
          libxl__device_backend_path(gc, dev));
 
     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);
-        /*
-         * Clone the libxl__device to avoid races if remove_device is called
-         * before the device addition has finished.
-         */
-        GCNEW(aodev->dev);
-        *aodev->dev = *dev;
-        aodev->action = LIBXL__DEVICE_ACTION_ADD;
-        aodev->callback = device_complete;
-        libxl__wait_device_connection(egc, aodev);
-
-        break;
     case LIBXL__DEVICE_KIND_QDISK:
         if (dguest->num_qdisks == 0) {
             GCNEW(dmss);
@@ -1599,10 +1580,19 @@  static int add_device(libxl__egc *egc, libxl__ao *ao,
             libxl__spawn_qdisk_backend(egc, dmss);
         }
         dguest->num_qdisks++;
-
         break;
     default:
-        rc = 1;
+        GCNEW(aodev);
+        libxl__prepare_ao_device(ao, aodev);
+        /*
+         * Clone the libxl__device to avoid races if remove_device is called
+         * before the device addition has finished.
+         */
+        GCNEW(aodev->dev);
+        *aodev->dev = *dev;
+        aodev->action = LIBXL__DEVICE_ACTION_ADD;
+        aodev->callback = device_complete;
+        libxl__wait_device_connection(egc, aodev);
         break;
     }
 
@@ -1619,11 +1609,17 @@  static int remove_device(libxl__egc *egc, libxl__ao *ao,
     int rc = 0;
 
     switch(ddev->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--;
-
+    case LIBXL__DEVICE_KIND_QDISK:
+        if (--dguest->num_qdisks == 0) {
+            rc = libxl__destroy_qdisk_backend(gc, dev->domid);
+            if (rc)
+                goto out;
+        }
+        libxl__device_destroy(gc, dev);
+        /* Return > 0, no ao has been dispatched */
+        rc = 1;
+        break;
+    default:
         GCNEW(aodev);
         libxl__prepare_ao_device(ao, aodev);
         /*
@@ -1636,17 +1632,6 @@  static int remove_device(libxl__egc *egc, libxl__ao *ao,
         aodev->callback = device_complete;
         libxl__initiate_device_generic_remove(egc, aodev);
         break;
-    case LIBXL__DEVICE_KIND_QDISK:
-        if (--dguest->num_qdisks == 0) {
-            rc = libxl__destroy_qdisk_backend(gc, dev->domid);
-            if (rc)
-                goto out;
-        }
-        libxl__device_destroy(gc, dev);
-        /* Fall through to return > 0, no ao has been dispatched */
-    default:
-        rc = 1;
-        break;
     }
 
     /*