From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore Date: Tue, 10 Feb 2015 11:49:50 +0000 Message-ID: <20150210114950.GF27856@zion.uk.xensource.com> References: <1423488068-31268-1-git-send-email-wei.liu2@citrix.com> <1423488068-31268-4-git-send-email-wei.liu2@citrix.com> <21721.58650.576986.983371@mariner.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: <21721.58650.576986.983371@mariner.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 Jackson Cc: jfehlig@suse.com, Wei Liu , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, Feb 10, 2015 at 11:01:46AM +0000, Ian Jackson wrote: > Wei Liu writes ("[PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore"): > > ... if backend is not set by caller. > > Acked-by: Ian Jackson > > as far as it goes, but I think you may want a more radical change - > see below. > > > Also change the function to use "goto" idiom while I was there. > > (Although usually it would be better to split this kind of thing into > a pre-patch, in this case it's small and easily reviewed.) > > Is the backend type the only missing or potentially-wrong > information ? ISTM that perhaps the caller might not know the target, > either. > > What should happen if the caller specifies a different target in disk > to the one the device is actually using ? The documentation should > specify which of the fields are important. > I'm not sure because it's not documented. We should take a step back to define the important fields first. > Maybe libxl_device_disk_remove needs to call libxl_vdev_to_device_disk > and check that the supplied disk struct is plausible somehow. In that > case it might be nice for the caller to be able to fill in only the > vdev. > If so we need to make clear in the documentation. I'm of course fine with this behaviour. Jim, does libvirt (as an example of libxl user) actually cares specifying every fields in that struct? The other user (xl) doesn't seem to care that much. Wei. > Ian.