Re: [PATCH v6] libxl: add option for discard support to xl disk configuration

[Olaf Hering <olaf@aepfle.de>]

On Mon, Jun 16, Ian Campbell wrote:

> I'm seeing an abort with this change while running under libvirt,
> although I think it is independent of the toolstack. It corresponds to
>         flexarray_append_pair(back, "discard-enable",
>                               libxl_defbool_val(disk->discard_enable) ?
>                               "1" : "0");
> in device_disk_add(). There is a missing libxl_defbool_setdefault in
> libxl somewhere and disk->discard_enable is therefore not set to a
> concrete value.
> 
> The one added by this patch in xlu_disk_parse is not sufficient because
> one is not obliged to use that function to parse disks. Although I also
> think it is wrong for it to be done there -- it should be done in the
> core libxl library somewhere, libxl__device_disk_setdefault most likely.

If the value is set via device_disk_add -> libxl__device_disk_setdefault
then the parsed value will be overwritten. Are you perhps thinking of
something like "if (!default(val)) set_default(val, 0|1);" in
libxl__device_disk_setdefault?

> The other alternative is that the xs key should simply be omitted unless
> the defbool is set to a specific value.

It was done that way in my initial implementation. IanJ suggested to set
a default, which is now done in xlu_disk_parse. As you found out, this
does not cover all cases. libxl__device_disk_add,
libxl_domain_create_new and libxl_domain_create_restore are public
interfaces which will end up in device_disk_add.

This untested change may be the way to fix the crash:

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b115df8..24d9ac7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2292,6 +2292,9 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
 {
     int rc;
 
+    if (!libxl_defbool_is_default(disk->discard_enable))
+        libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);
+
     rc = libxl__resolve_domid(gc, disk->backend_domname, &disk->backend_domid);
     if (rc < 0) return rc;
 
diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
index 752a2c7..18fe386 100644
--- a/tools/libxl/libxlu_disk.c
+++ b/tools/libxl/libxlu_disk.c
@@ -79,7 +79,6 @@ int xlu_disk_parse(XLU_Config *cfg,
         if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
             disk->format = LIBXL_DISK_FORMAT_EMPTY;
     }
-    libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);
 
     if (!disk->vdev) {
         xlu__disk_err(&dpc,0, "no vdev specified");


Olaf