From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions Date: Mon, 2 Feb 2015 13:36:00 +0000 Message-ID: <1422884160.19293.23.camel@citrix.com> References: <54C6CA4A.4040505@suse.com> <1422883633.19293.19.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1422883633.19293.19.camel@citrix.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: Jim Fehlig Cc: Ian Jackson , Wei Liu , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On Mon, 2015-02-02 at 13:27 +0000, Ian Campbell wrote: > On Mon, 2015-01-26 at 16:14 -0700, Jim Fehlig wrote: > > Cc-ing the other toolstack maintainers, both of whom have more > familiarity with this part of libxl than I. > > > The attached patch is a hack I cooked up to fix one of the libvirt-TCK > > Xen failures. The test (200-disk-hotplug.t) attempts to hot add and > > remove a disk from a running domain. The add works fine, but remove > > fails with > > > > libxl: debug: libxl.c:3858:libxl_device_disk_remove: ao 0x7f9b9c0015a0: > > create: how=(nil) callback=(nil) poller= > > 0x7f9ba0004590 > > libxl: error: libxl.c:2399:libxl__device_from_disk: unrecognized disk > > backend type: 0 > > > > The test does not define a backend type, in which case the libvirt libxl > > driver allows libxl to choose an appropriate backend via > > libxl__device_disk_set_backend(). The backend type is never set on a > > remove operation, hence it fails with the above error. > > > > I spent some time trying to figure out the best place to set backend > > type on remove, but in the end could only come up with this hack. It > > wouldn't feel so gross if I could have simply added > > 'libxl__device_##type##_setdefault(gc, type);' to the existing > > DEFINE_DEVICE_REMOVE macro, but alas libxl__device_nic_setdefault() has > > a different prototype than the other devices. (I'm not sure this is the right apporach, but if it were...) FWIW libxl__ functions aren't stable, so in theory this could be changed to rationalise them all (e.g. by adding domid to the rest), but that would be a bit churnful. Perhaps better would be to add a new parameter to0 DEFINE_DEVICE_REMOVE like extra_setdefault_args which is pasted in the appropriate place? > > Better suggestions welcomed! One I considered was fixing this in > > libvirt. But the Xen community suggested allowing libxl to choose a > > suitable backend when not specified, so I think this recommendation > > should be symmetrical in the add and remove operations. I suppose on remove its not so much a case of choosing a suitable backend as reflecting the actual current reality for that device. Which suggests that libxl__device_from_disk ought to be figuring out the actual backend somehow rather than being told it. Ian.