[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH 10/20: switch QEMU to using the driver API



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 redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]