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 -=|