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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list