On Fri, Jun 22, 2007 at 02:56:58AM +0100, Daniel P. Berrange wrote:
> This is the 2nd key patch in the series. It switches the qemud/driver.c
> file methods to all follow the official libvirt internal driver API.
> NB, since we're using dummy statically declared virConnect object in
> the dispatch.c file, we can't use the regular virGetDomain function
> for constructing virDomainPtr objects at this time. Thus we have a
> hack to just malloc a virDomain struct & manually fill in the fields.
> This hack will go away in a later patch.
Okay, just two nitpicks, one about lack of comments, okay those
functions are basically described as part of the driver API so it's a
bit like cut and paste, but I still think it's nicer to have next to
the code, and some lines are really long,
> - for (i = 0 ; i < QEMUD_MAX_NUM_NETWORKS ; i++) {
> - names[i] = &out->qemud_packet_server_data_u.listNetworksReply.networks[i*QEMUD_MAX_NAME_LEN];
> - }
There is a lot of place like that where the code manipulates
all the dereference chain a lot of time, why not use temporary
variables to make the code more readable and help the compiler
w.r.t. generating good code ?
+1 but maybe we need to revisit a bit that code.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/