From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore Date: Tue, 10 Feb 2015 11:01:46 +0000 Message-ID: <21721.58650.576986.983371@mariner.uk.xensource.com> References: <1423488068-31268-1-git-send-email-wei.liu2@citrix.com> <1423488068-31268-4-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1423488068-31268-4-git-send-email-wei.liu2@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: Wei Liu Cc: jfehlig@suse.com, Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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. 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. Ian.