[libvirt] [PATCH]: Rework xenLinuxDomainDeviceID to support more devices

[Daniel P. Berrange]

On Tue, Aug 05, 2008 at 02:08:24PM +0200, Chris Lalancette wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 05, 2008 at 12:12:21PM +0200, Chris Lalancette wrote:
> >> Recently upstream Xen added support for having xvd devices > 16.  For the most
> >> part, this doesn't really concern libvirt, since for things like attach and
> >> detach we just pass it through and let xend worry about whether it is supported
> >> or not.  The one place this breaks down is in the stats collecting code, where
> >> we need to figure out the device number so we can go digging in /sys for the
> >> statistics.
> >>
> >> To remedy this, I've re-written xenLinuxDomainDeviceID() to use regular
> >> expressions to figure out the device number from the name.  The major advantage
> >> is that now xenLinuxDomainDeviceID() looks fairly identical to
> >> tools/python/xen/util/blkif.py (in the Xen sources), so that adding additional
> >> devices in the future should be much easier.  It also reduces the size of the
> >> code, and, in my opinion, the code complexity.
> >>
> >> With this patch in place, I was able to get block statistics both on older style
> >> devices (/dev/xvda) and on the new, expanded devices (/dev/xvdaa).
> > 
> > This patch breaks the test suite for disk name -> device ID conversion.
> > The test suite also needs to have more  tests added to cover the new 
> > interesting boundary conditions for xvdXXX naming.
> 
> OK.  Well, there were 3 different problems with the test suite:
> 
> 1)  A number of tests were actually wrong.  For instance, there is a
> DO_TEST("/dev/hdt", 23359); but Xen actually uses the encoding "major*256 +
> minor + part".  So in this case, the major is 91, and the minor is 64 (according
> to http://www.lanana.org/docs/device-list/devices.txt), so that would be 23360.
>  I've fixed the wrong tests now, and I'll re-submit it with the updated patch.
> 
> 2)  In the hda0 and hda64 case, the upstream Xen regex isn't tight enough.  I've
> tightened it up in the libvirt patch, so these now pass.
> 
> 3)  The upstream Xen regex's allows /dev/sdi{w,x,y,z}, although they aren't
> legal devices.  I've fixed up my regex to handle this.
> 
> Attached is an updated patch with the above fixes both to my code and to the
> test suite.  As far as the error reporting goes, I won't argue that my patch
> gives slightly less information.  However, that being said, I have to believe
> that the most likely use of block statistics is something like:

I'd like to see a patch which does an hybrid of the existing vs regex
approach. ie, use a single regex to split the string into disk prefix, 
disk number, and partition number. Then do validation on the ranges 
and report errors accordingly.

> virsh dumpxml <dom>
> ...see what devices are listed there
> virsh domblkstats <dom> <device>
> 
> In which case the slightly less verbose error reporting won't matter a whole lot.

Error reporting shouldn't be done based on the based on a particular
usage scenario. Any API call should aim to give error messages that
are detailed enough to understand the actual problem without needing
further context.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|