[Libvir] [PATCH] Refactor xend_internal.c block device detection
This patch is a little bit esoteric, but I need it for something I'm working on at the moment. At the moment the code in xend_internal.c: xend_parse_sexp_desc parses the domain sexpr directly into XML. This makes it rather hard to just get a list of block devices without repeating the same code. So here I've factored out the common code for parsing block devices into a separate function and callback. There are a couple of small changes that you should be aware of: (1) The <devices> list may be returned in a different order (specifically, block devices are always returned first). (2) We iterate over the root nodes of the sexpr twice (once for block devices, once for vifs and vfbs). But the sexpr is small and in-memory so this shouldn't be a problem compared to having to do the HTTP request to xend to get it in the first place. 'make check' passes. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Oct 16, 2007 at 02:13:04PM +0100, Richard W.M. Jones wrote:
This patch is a little bit esoteric, but I need it for something I'm working on at the moment.
At the moment the code in xend_internal.c: xend_parse_sexp_desc parses the domain sexpr directly into XML. This makes it rather hard to just get a list of block devices without repeating the same code. So here I've factored out the common code for parsing block devices into a separate function and callback.
You really need a callback ? This makes code harder to understand
+ /* Call the callback function. */ + ret = fn (conn, data, isBlock, cdrom, isNoSrcCdrom, + drvName, drvType, src, dst, mode);
This adds flexibility but is a bit convoluted, what do you need this for ?
There are a couple of small changes that you should be aware of: (1) The <devices> list may be returned in a different order (specifically, block devices are always returned first). (2) We iterate over the root nodes of the sexpr twice (once for block devices, once for vifs and vfbs). But the sexpr is small and in-memory so this shouldn't be a problem compared to having to do the HTTP request to xend to get it in the first place.
Having disks presented first in the resulting XML is a bit cleaner IMHO and I don't think the double scan of teh sexpr is a big deal, really. I'm more wondering about what you need to achieve that really needs a callback based interface. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Oct 16, 2007 at 02:13:04PM +0100, Richard W.M. Jones wrote:
This patch is a little bit esoteric, but I need it for something I'm working on at the moment.
At the moment the code in xend_internal.c: xend_parse_sexp_desc parses the domain sexpr directly into XML. This makes it rather hard to just get a list of block devices without repeating the same code. So here I've factored out the common code for parsing block devices into a separate function and callback.
I agree that its good to separate this code out, however, I'd split it slightly differently. Instead of a function to parse all block devices and a callback to receive each one, I'd have the new function just be responsible for parsing a single block device. Keep iteration over block devices in the caller & avoid the callback. This is more in keeping with the style of the other functions for parsing XML & related domain info formats Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
On Tue, Oct 16, 2007 at 02:13:04PM +0100, Richard W.M. Jones wrote:
This patch is a little bit esoteric, but I need it for something I'm working on at the moment.
At the moment the code in xend_internal.c: xend_parse_sexp_desc parses the domain sexpr directly into XML. This makes it rather hard to just get a list of block devices without repeating the same code. So here I've factored out the common code for parsing block devices into a separate function and callback.
I agree that its good to separate this code out, however, I'd split it slightly differently. Instead of a function to parse all block devices and a callback to receive each one, I'd have the new function just be responsible for parsing a single block device. Keep iteration over block devices in the caller & avoid the callback. This is more in keeping with the style of the other functions for parsing XML & related domain info formats
I'm not sure I understand. Do you mean the function would parse a block device and return a structure? Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Oct 16, 2007 at 03:16:34PM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
On Tue, Oct 16, 2007 at 02:13:04PM +0100, Richard W.M. Jones wrote:
This patch is a little bit esoteric, but I need it for something I'm working on at the moment.
At the moment the code in xend_internal.c: xend_parse_sexp_desc parses the domain sexpr directly into XML. This makes it rather hard to just get a list of block devices without repeating the same code. So here I've factored out the common code for parsing block devices into a separate function and callback.
I agree that its good to separate this code out, however, I'd split it slightly differently. Instead of a function to parse all block devices and a callback to receive each one, I'd have the new function just be responsible for parsing a single block device. Keep iteration over block devices in the caller & avoid the callback. This is more in keeping with the style of the other functions for parsing XML & related domain info formats
I'm not sure I understand. Do you mean the function would parse a block device and return a structure?
Yes, that's what I was thinking, although I forgot this was in the Xen driver, not QEMU, and so doesn't have any such struct defined at this time. So perhaps its not worth the hassle - go with your original patch if you like. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones