On Fri, Jun 22, 2007 at 08:45:59AM -0400, Daniel Veillard wrote:
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 ?
Again the dispatch.c file is killed in the last patch, so I wasn't really focusing on making nice code in dispatch.c Just functional enough for me to verify it works after each patch in the series is applied, so I didn't waste too much effort on code that is going away :-)
+1 but maybe we need to revisit a bit that code.
Yep, by deleting the entire file :-) 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 -=|