From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olaf Hering Subject: Re: [PATCH v6] libxl: add option for discard support to xl disk configuration Date: Tue, 17 Jun 2014 08:36:35 +0200 Message-ID: <20140617063635.GA10777@aepfle.de> References: <1400493019-18114-1-git-send-email-olaf@aepfle.de> <21369.61499.949523.549356@mariner.uk.xensource.com> <1402936023.23566.14.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1402936023.23566.14.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: stefano.stabellini@eu.citrix.com, Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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