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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-06-17 at 08:36 +0200, Olaf Hering wrote:
> 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?

This is exactly what libxl_defbool_setdefault does/is for.

> > 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.

OK.

>  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);

No need for the condition here, setdefault already incorporates it.

I think with that changed this is probably the right fix.

> +
>      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



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