Re: [Libvir] PATCH 10/20: switch QEMU to using the driver API
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 -=|