From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 5/6] libxl: introduce libxl__alloc_vdev Date: Fri, 30 Mar 2012 12:57:18 +0100 Message-ID: References: <1332856772-30292-5-git-send-email-stefano.stabellini@eu.citrix.com> <20337.63133.683848.208682@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20337.63133.683848.208682@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: "xen-devel@lists.xensource.com" , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Tue, 27 Mar 2012, Ian Jackson wrote: > Stefano Stabellini writes ("[PATCH 5/6] libxl: introduce libxl__alloc_vdev"): > > Introduce libxl__alloc_vdev: find a spare virtual block device in the > > domain passed as argument. > ... > > +static int libxl__vdev_to_index(libxl__gc *gc, char *vdev) > > +{ > > + if (vdev == NULL) > > + return 0; > > + if (strlen(vdev) > 4 || strlen(vdev) < 4) > > + return 0; > > + /* assume xvdz is the last one available */ > > This is a broken half-reimplementation of > libxl__device_disk_dev_number. No it is not the same thing. This function doesn't return any meaningful number representing the disk in a unique way, it basically returns the last letter of the disk name as an integer (minus offset). > > + free(disks); > > + return libxl__index_to_vdev(gc, max_idx); > > And you don't need this function because you can use the disk number > directly (converted to a string of decimal digits) as the vdev. These two functions are not the same as libxl__device_disk_dev_number and friends. They are just helper functions for libxl__alloc_vdev. I could rename them libxl__vdev_helper_idx and libxl__vdev_helper_name to avoid confusion. > Of course you can't then using the resulting vdev as a pathname in > /dev/ but that's nonportable anyway; different guest OSs (and here we > are playing the role of the guest) may have different conventions. > You need a separate function to convert vdev numbers (as returned from > _disk_dev_number) to pathnames in /dev. This is a good point and the key issue here. The frontend is actually free to choose whatever name it wants for the device it is going to create from the dev number. We don't have a proper way to get this device name, the closest thing we have is libxl__device_disk_dev_number that from a vdev returns the dev number. So I think that the best thing we can do is rely on it to find out the device name, either doing: [1] xvd(a + i) -> libxl__device_disk_dev_number -> xs_read or reading the "dev" node under the xenstore backend path, that is generated using the same libxl function (this is what I am doing now). I fail to see why one of these two approaches would be better than the other, but if you think that [1] is the best, I can change my code. > > + for (i = 0; i < num; i++) { > > + idx = libxl__vdev_to_index(gc, disks[i].vdev); > > Furthermore, for the purpose for which you're using this, you should > not start at zero (xvda). You should start at at least 26 (xvdA) and > possibly later. That way you are less likely to clash with other > statically-allocated vbds. Another interesting observation but I disagree: why should we expect "statically-allocated vbds" below xvdA? How many do you think we are going to find? Why 26? I think it is very arbitrary. We should just make sure that our allocation policy works well and start from 0.