Skip to contentSkip to conversation

libxl: refactor tap disk handling

PATCH In review Ian Campbell 3 participants 11 messages Apr 7, 2011 – Apr 8, 2011

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 libxl: refactor tap disk handling

I'm not personally convinced that support for blktap2 devices should be conflated in libxl with the PV block backend support but given it's there lets at least correct it.

In a blktap2 system there is no "tap" PV backend type. blktap2 exposes a standard block device and this is passed to a guest using the standard blkback "vbd" (sometimes called "phy") backend. Drop the "tap" backend type and associated (libxl internal ) DEVICE_TAP enumeration value.

Also try and clarify the paths which fallback from blktap2 to qdisk when the former is not present. Falling through a switch statement is a neat way of doing this in some cases, but I don't think this is one of them.

#1 lore raw

PATCH 1/1 libxl: remove impossible check for backend != DISK_BACKEND_QDISK

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1302167172 -3600
# Node ID 29af1669de3ab2e7d41078eb33a0dfa9b69f09bb
# Parent bbb1ffd81018b0b7677c8b526de118d4501f6d39
libxl: remove impossible check for backend != DISK_BACKEND_QDISK

In this case we are already in the DISK_BACKEND_QDISK case of a switch statement on the same variable.

It is possible that we fell through from the DISK_BACKEND_TAP case (although I'm about to remove that in a subsequent patch), however in that case we are explicitly falling back from blktap2 to qdisk so DEVICE_QDISK is still the right answer.

tools/libxl/libxl.c +1 -6
diff -r bbb1ffd81018 -r 29af1669de3a tools/libxl/libxl.c
@@ -1016,12 +1016,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
             flexarray_append(back, "params");
             flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
                           libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
-
-            if (libxl__blktap_enabled(&gc) && 
-                 disk->backend != DISK_BACKEND_QDISK)
-                device.backend_kind = DEVICE_TAP;
-            else
-                device.backend_kind = DEVICE_QDISK;
+            device.backend_kind = DEVICE_QDISK;
             break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
#2 lore raw

PATCH 1/1 libxl: make fallback from blktap2 to qdisk more explicit

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1302167195 -3600
# Node ID 76d363a113e5ee6111bc9e0eca5a314644ce6295 # Parent 29af1669de3ab2e7d41078eb33a0dfa9b69f09bb
libxl: make fallback from blktap2 to qdisk more explicit.

When blktap2 is not present we fallback to qdisk, instead of falling through a switch statement instead make this explicit, with a comment, prior to the switch statement.

tools/libxl/libxl.c +7 -2
diff -r 29af1669de3a -r 76d363a113e5 tools/libxl/libxl.c
@@ -979,6 +979,10 @@ int libxl_device_disk_add(libxl_ctx *ctx
     device.domid = domid;
     device.kind = DEVICE_VBD;
 
+    /* If blktap is not available then fallback to qdisk */
+    if (disk->backend == DISK_BACKEND_TAP && !libxl__blktap_enabled(&gc))
+        disk->backend = DISK_BACKEND_QDISK;
+
     switch (disk->backend) {
         case DISK_BACKEND_PHY: 
             libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor);
@@ -991,7 +995,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
             device.backend_kind = DEVICE_VBD;
             break;
         case DISK_BACKEND_TAP:
-            if (libxl__blktap_enabled(&gc) && disk->format != DISK_FORMAT_EMPTY) {
+            if (disk->format != DISK_FORMAT_EMPTY) {
                 const char *dev = libxl__blktap_devpath(&gc,
                                                disk->pdev_path, disk->format);
                 if (!dev) {
@@ -1012,7 +1016,8 @@ int libxl_device_disk_add(libxl_ctx *ctx
 
                 break;
             }
-        case DISK_BACKEND_QDISK: 
+            break;
+        case DISK_BACKEND_QDISK:
             flexarray_append(back, "params");
             flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
                           libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
#3 lore raw

PATCH 1/1 libxl: convert an empty tap disk into a qdisk

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1302167196 -3600
# Node ID f5418873be3d097cbb374009898ce9c74255e047 # Parent 76d363a113e5ee6111bc9e0eca5a314644ce6295
libxl: convert an empty tap disk into a qdisk

I'm not sure that empty disks which are is_cdrom are especially valid, or that a cdrom can ever be handled by tapdisk anyway but try to do something sane since it seems that xl's parse_disk_config() routine could potentially generate such a configuration (although whether from a valid input string or not I'm not sure).

tools/libxl/libxl.c +11 -5
diff -r 76d363a113e5 -r f5418873be3d tools/libxl/libxl.c
@@ -983,6 +983,14 @@ int libxl_device_disk_add(libxl_ctx *ctx
     if (disk->backend == DISK_BACKEND_TAP && !libxl__blktap_enabled(&gc))
         disk->backend = DISK_BACKEND_QDISK;
 
+    /*
+     * blktap cannot handle empty disks (aka cdroms). Fall back to
+     * qdisk because qemu-xen creates the disk based on the xenstore
+     * entries.
+     */
+    if (disk->backend == DISK_BACKEND_TAP && disk->format == DISK_FORMAT_EMPTY)
+        disk->backend == DISK_BACKEND_QDISK;
+
     switch (disk->backend) {
         case DISK_BACKEND_PHY: 
             libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor);
@@ -995,7 +1003,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
             device.backend_kind = DEVICE_VBD;
             break;
         case DISK_BACKEND_TAP:
-            if (disk->format != DISK_FORMAT_EMPTY) {
+            {
                 const char *dev = libxl__blktap_devpath(&gc,
                                                disk->pdev_path, disk->format);
                 if (!dev) {
@@ -1003,8 +1011,8 @@ int libxl_device_disk_add(libxl_ctx *ctx
                     goto out_free;
                 }
                 flexarray_append(back, "tapdisk-params");
-                flexarray_append(back, libxl__sprintf(&gc, "%s:%s", 
-                    libxl__device_disk_string_of_format(disk->format), 
+                flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
+                    libxl__device_disk_string_of_format(disk->format),
                     disk->pdev_path));
                 flexarray_append(back, "params");
                 flexarray_append(back, libxl__strdup(&gc, dev));
@@ -1013,8 +1021,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
                 flexarray_append(back, "physical-device");
                 flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
                 device.backend_kind = DEVICE_VBD;
-
-                break;
             }
             break;
         case DISK_BACKEND_QDISK:
#4 lore raw

PATCH 1/1 libxl: only a CDROM type disk can be empty

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1302167211 -3600
# Node ID cbf6ec01faef6502845d8201a717edc9fa6fae52 # Parent f5418873be3d097cbb374009898ce9c74255e047
libxl: only a CDROM type disk can be empty.

tools/libxl/libxl.c +7 -2
diff -r f5418873be3d -r cbf6ec01faef tools/libxl/libxl.c
@@ -908,8 +908,13 @@ static int validate_virtual_disk(libxl__
     struct stat stat_buf;
     char *delimiter;
 
-    if (disk->format == DISK_FORMAT_EMPTY)
-        return 0;
+    if (disk->format == DISK_FORMAT_EMPTY) {
+        if (disk->is_cdrom)
+            return 0;
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Empty disk %s is not a CDROM device\n",
+                   disk->vdev);
+        return ERROR_INVAL;
+    }
 
     if (disk->format == DISK_FORMAT_RAW) {
         delimiter = strchr(file_name, ':');
#5 lore raw

PATCH 1/1 libxl: refactor DISK_BACKEND_PHY handling in libxl_device_disk_add

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1302167211 -3600
# Node ID ec3b25b5aa8095c5fe070173af888f032f6c2c5e
# Parent cbf6ec01faef6502845d8201a717edc9fa6fae52
libxl: refactor DISK_BACKEND_PHY handling in libxl_device_disk_add

A step on the path to sharing this code with the tail-end of the DISK_BACKEND_TAP case.

I made the result of libxl__blktap_devpath non-const to achieve this. The existing caller calls libxl__strdup on the result but since the function is an internal one and the result is already garbage collected I think this is unnecessary and we can just use the non-const result directly.

tools/libxl/libxl.c +26 -22
diff -r cbf6ec01faef -r ec3b25b5aa80 tools/libxl/libxl.c
@@ -949,6 +949,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
     libxl__gc gc = LIBXL_INIT_GC(ctx);
     flexarray_t *front;
     flexarray_t *back;
+    char *dev;
     char *backend_type;
     int devid;
     libxl__device device;
@@ -997,36 +998,39 @@ int libxl_device_disk_add(libxl_ctx *ctx
         disk->backend == DISK_BACKEND_QDISK;
 
     switch (disk->backend) {
-        case DISK_BACKEND_PHY: 
-            libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor);
+        case DISK_BACKEND_PHY:
+            dev = disk->pdev_path;
+
+            libxl__device_physdisk_major_minor(dev, &major, &minor);
             flexarray_append(back, "physical-device");
             flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
 
             flexarray_append(back, "params");
-            flexarray_append(back, disk->pdev_path);
+            flexarray_append(back, dev);
 
             device.backend_kind = DEVICE_VBD;
             break;
         case DISK_BACKEND_TAP:
-            {
-                const char *dev = libxl__blktap_devpath(&gc,
-                                               disk->pdev_path, disk->format);
-                if (!dev) {
-                    rc = ERROR_FAIL;
-                    goto out_free;
-                }
-                flexarray_append(back, "tapdisk-params");
-                flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
-                    libxl__device_disk_string_of_format(disk->format),
-                    disk->pdev_path));
-                flexarray_append(back, "params");
-                flexarray_append(back, libxl__strdup(&gc, dev));
-                backend_type = "phy";
-                libxl__device_physdisk_major_minor(dev, &major, &minor);
-                flexarray_append(back, "physical-device");
-                flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
-                device.backend_kind = DEVICE_VBD;
+            dev = libxl__blktap_devpath(&gc, disk->pdev_path, disk->format);
+            if (!dev) {
+                rc = ERROR_FAIL;
+                goto out_free;
             }
+            flexarray_append(back, "tapdisk-params");
+            flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
+                libxl__device_disk_string_of_format(disk->format),
+                disk->pdev_path));
+
+            flexarray_append(back, "params");
+            flexarray_append(back, dev);
+
+            backend_type = "phy";
+
+            libxl__device_physdisk_major_minor(dev, &major, &minor);
+            flexarray_append(back, "physical-device");
+            flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
+
+            device.backend_kind = DEVICE_VBD;
             break;
         case DISK_BACKEND_QDISK:
             flexarray_append(back, "params");
@@ -1110,7 +1114,7 @@ int libxl_device_disk_del(libxl_ctx *ctx
 char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    const char *dev = NULL;
+    char *dev = NULL;
     char *ret = NULL;
 
     switch (disk->backend) {
tools/libxl/libxl_blktap2.c +3 -3
diff -r cbf6ec01faef -r ec3b25b5aa80 tools/libxl/libxl_blktap2.c
@@ -24,9 +24,9 @@ int libxl__blktap_enabled(libxl__gc *gc)
     return !tap_ctl_check(&msg);
 }
 
-const char *libxl__blktap_devpath(libxl__gc *gc,
-                                 const char *disk,
-                                 libxl_disk_format format)
+char *libxl__blktap_devpath(libxl__gc *gc,
+                            const char *disk,
+                            libxl_disk_format format)
 {
     const char *type;
     char *params, *devname = NULL;
tools/libxl/libxl_internal.h +3 -3
diff -r cbf6ec01faef -r ec3b25b5aa80 tools/libxl/libxl_internal.h
@@ -325,9 +325,9 @@ _hidden int libxl__blktap_enabled(libxl_
  *    returns device path xenstore wants to have. returns NULL
  *      if no device corresponds to the disk.
  */
-_hidden const char *libxl__blktap_devpath(libxl__gc *gc,
-                                 const char *disk,
-                                 libxl_disk_format format);
+_hidden char *libxl__blktap_devpath(libxl__gc *gc,
+                                    const char *disk,
+                                    libxl_disk_format format);
 
 _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
 
tools/libxl/libxl_noblktap2.c +3 -3
diff -r cbf6ec01faef -r ec3b25b5aa80 tools/libxl/libxl_noblktap2.c
@@ -21,9 +21,9 @@ int libxl__blktap_enabled(libxl__gc *gc)
     return 0;
 }
 
-const char *libxl__blktap_devpath(libxl__gc *gc,
-                                 const char *disk,
-                                 libxl_disk_format format)
+char *libxl__blktap_devpath(libxl__gc *gc,
+                            const char *disk,
+                            libxl_disk_format format)
 {
     return NULL;
 }
#6 lore raw

PATCH 1/1 libxl: handle the tail end of a tap device using the phy backend handling code

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1302167211 -3600
# Node ID 40b0954cb5d9bdd0b943963afb79914ba5cb08c9
# Parent ec3b25b5aa8095c5fe070173af888f032f6c2c5e
libxl: handle the tail end of a tap device using the phy backend handling code

We are literally creating a phy backend on top of a blktap2 created device anyway so we might as well reuse the code and make this explicit.

tools/libxl/libxl.c +4 -10
diff -r ec3b25b5aa80 -r 40b0954cb5d9 tools/libxl/libxl.c
@@ -1000,7 +1000,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
     switch (disk->backend) {
         case DISK_BACKEND_PHY:
             dev = disk->pdev_path;
-
+    do_backend_phy:
             libxl__device_physdisk_major_minor(dev, &major, &minor);
             flexarray_append(back, "physical-device");
             flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
@@ -1021,17 +1021,11 @@ int libxl_device_disk_add(libxl_ctx *ctx
                 libxl__device_disk_string_of_format(disk->format),
                 disk->pdev_path));
 
-            flexarray_append(back, "params");
-            flexarray_append(back, dev);
-
             backend_type = "phy";
 
-            libxl__device_physdisk_major_minor(dev, &major, &minor);
-            flexarray_append(back, "physical-device");
-            flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
-
-            device.backend_kind = DEVICE_VBD;
-            break;
+            /* now create a phy device to export the device to the guest */
+            goto do_backend_phy;
+
         case DISK_BACKEND_QDISK:
             flexarray_append(back, "params");
             flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
#7 lore raw

PATCH 1/1 libxl: Drop internal DEVICE_TAP backend type

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1302167211 -3600
# Node ID 6162ae4cc9a7fae505f6b081a6698ecdd94418c2 # Parent 40b0954cb5d9bdd0b943963afb79914ba5cb08c9
libxl: Drop internal DEVICE_TAP backend type

There is no such thing with blktap2, the backend in that case is PHY.

libxl_device_disk_del was just plain wrong in this regard, fix it to select the appropriate backend_kind.

tools/libxl/libxl.c +19 -7
diff -r 40b0954cb5d9 -r 6162ae4cc9a7 tools/libxl/libxl.c
@@ -950,7 +950,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
     flexarray_t *front;
     flexarray_t *back;
     char *dev;
-    char *backend_type;
     int devid;
     libxl__device device;
     int major, minor, rc;
@@ -970,7 +969,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
         goto out_free;
     }
 
-    backend_type = libxl__device_disk_string_of_backend(disk->backend);
     devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
     if (devid==-1) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
@@ -1021,8 +1019,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
                 libxl__device_disk_string_of_format(disk->format),
                 disk->pdev_path));
 
-            backend_type = "phy";
-
             /* now create a phy device to export the device to the guest */
             goto do_backend_phy;
 
@@ -1051,7 +1047,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
     flexarray_append(back, "dev");
     flexarray_append(back, disk->vdev);
     flexarray_append(back, "type");
-    flexarray_append(back, backend_type);
+    flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
     flexarray_append(back, "mode");
     flexarray_append(back, disk->readwrite ? "w" : "r");
 
@@ -1093,14 +1089,30 @@ int libxl_device_disk_del(libxl_ctx *ctx
     devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
     device.backend_domid    = disk->backend_domid;
     device.backend_devid    = devid;
-    device.backend_kind     =
-        (disk->backend == DISK_BACKEND_PHY) ? DEVICE_VBD : DEVICE_TAP;
+
+    switch (disk->backend) {
+        case DISK_BACKEND_PHY:
+            device.backend_kind = DEVICE_VBD;
+            break;
+        case DISK_BACKEND_TAP:
+            device.backend_kind = DEVICE_VBD;
+            break;
+        case DISK_BACKEND_QDISK:
+            device.backend_kind = DEVICE_QDISK;
+            break;
+        default:
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
+                       disk->backend);
+            rc = ERROR_INVAL;
+            goto out_free;
+    }
     device.domid            = domid;
     device.devid            = devid;
     device.kind             = DEVICE_VBD;
     rc = libxl__device_del(&gc, &device, wait);
     xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device));
     xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device));
+out_free:
     libxl__free_all(&gc);
     return rc;
 }
tools/libxl/libxl_device.c +1 -2
diff -r 40b0954cb5d9 -r 6162ae4cc9a7 tools/libxl/libxl_device.c
@@ -31,7 +31,6 @@
 static const char *string_of_kinds[] = {
     [DEVICE_VIF] = "vif",
     [DEVICE_VBD] = "vbd",
-    [DEVICE_TAP] = "tap",
     [DEVICE_QDISK] = "qdisk",
     [DEVICE_PCI] = "pci",
     [DEVICE_VFB] = "vfb",
@@ -135,7 +134,7 @@ char *libxl__device_disk_string_of_backe
 {
     switch (backend) {
         case DISK_BACKEND_QDISK: return "qdisk";
-        case DISK_BACKEND_TAP: return "tap";
+        case DISK_BACKEND_TAP: return "phy";
         case DISK_BACKEND_PHY: return "phy";
         default: return NULL;
     }
tools/libxl/libxl_internal.h +0 -1
diff -r 40b0954cb5d9 -r 6162ae4cc9a7 tools/libxl/libxl_internal.h
@@ -97,7 +97,6 @@ struct libxl__ctx {
 typedef enum {
     DEVICE_VIF = 1,
     DEVICE_VBD,
-    DEVICE_TAP,
     DEVICE_QDISK,
     DEVICE_PCI,
     DEVICE_VFB,
Applied
#8 lore raw

Re: [PATCH 7 of 7] libxl: Drop internal DEVICE_TAP backend type

Ian Campbell writes ("[Xen-devel] [PATCH 7 of 7] libxl: Drop internal DEVICE_TAP backend type"):

Quoted 1 line

libxl: Drop internal DEVICE_TAP backend type

Applied all seven, thanks.

Ian.

#9 lore raw
In reply to Ian Campbell
Quoted 9 lines · Ian Campbell

I'm not personally convinced that support for blktap2 devices should be conflated in libxl with the PV block backend support but given it's there lets at least correct it.

In a blktap2 system there is no "tap" PV backend type. blktap2 exposes a standard block device and this is passed to a guest using the standard blkback "vbd" (sometimes called "phy") backend. Drop the "tap" backend type and associated (libxl internal ) DEVICE_TAP enumeration value.

What happens on a no-blktap2 system?

Quoted 11 lines

Also try and clarify the paths which fallback from blktap2 to qdisk when the former is not present. Falling through a switch statement is a neat way of doing this in some cases, but I don't think this is one of them.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
#10 lore raw
Quoted 5 lines · Christoph Egger
Quoted 4 lines · Ian Campbell

I'm not personally convinced that support for blktap2 devices should be conflated in libxl with the PV block backend support but given it's there lets at least correct it.

Quoted 7 lines
Quoted 5 lines

In a blktap2 system there is no "tap" PV backend type. blktap2 exposes a standard block device and this is passed to a guest using the standard blkback "vbd" (sometimes called "phy") backend. Drop the "tap" backend type and associated (libxl internal ) DEVICE_TAP enumeration value.

What happens on a no-blktap2 system?

Same as before, libxl__blktap_enabled still returns false and the same things should happen as a result.

Ian.

Quoted 14 lines
Quoted 12 lines

Also try and clarify the paths which fallback from blktap2 to qdisk when the former is not present. Falling through a switch statement is a neat way of doing this in some cases, but I don't think this is one of them.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel