On Fri, Jun 22, 2007 at 07:16:19AM -0400, Daniel Veillard wrote:
On Fri, Jun 22, 2007 at 02:48:38AM +0100, Daniel P. Berrange wrote:
This is the 2nd biggest patch of the series. It is a major refactoring of the data structures, to split qemud_server into a smaller qemud_server and a new qemud_driver.
conf.c | 251 ++++++++++---------- conf.h | 294 +++++++++++++++++++++++- dispatch.c | 295 ++++++++++-------------- driver.c | 733 ++++++++++++++++++++++++++++++++++--------------------------- driver.h | 98 +++----- internal.h | 236 ------------------- qemud.c | 50 ---- 7 files changed, 992 insertions(+), 965 deletions(-)
[...]
diff -r 4684eb84957d qemud/conf.h --- a/qemud/conf.h Thu Jun 21 16:14:19 2007 -0400 +++ b/qemud/conf.h Thu Jun 21 16:14:44 2007 -0400 @@ -24,15 +24,277 @@ #ifndef __QEMUD_CONF_H #define __QEMUD_CONF_H [...] + +static inline int +qemudIsActiveVM(struct qemud_vm *vm) +{ + return vm->id != -1; +} + +static inline int +qemudIsActiveNetwork(struct qemud_network *network) +{ + return network->active; +}
I'm not too fond of this, this assumes a compiler accepting the inlining and or having code being defined in the config file. I way prefer a simple macro, that's more portable.
This was in the code already - I just moved it from internal.h to conf.h
diff -r 4684eb84957d qemud/dispatch.c --- a/qemud/dispatch.c Thu Jun 21 16:14:19 2007 -0400 +++ b/qemud/dispatch.c Thu Jun 21 16:14:44 2007 -0400 [...] +extern struct qemud_driver *qemu_driver;
hum, is that a declaration or ar reference ? I'm alway a bit suspicious of such construct, if shared it goes in the .h as extern, and without extern in one .c , if not shared it should really be static.
It is a reference. It is also an evil short term hack. The entire dispatch.c file is killed off in patch 20, so can ignore it.
[...]
diff -r 4684eb84957d qemud/driver.c --- a/qemud/driver.c Thu Jun 21 16:14:19 2007 -0400 +++ b/qemud/driver.c Thu Jun 21 16:26:12 2007 -0400 @@ -23,6 +23,8 @@
#include <config.h>
+#define _GNU_SOURCE /* for asprintf */ +
please no, I understand it's painful and error prone to do the calculation of the target string lenght, but that's really not portable.
Again just copying existing code here.
[...]
+ if (asprintf (&base, "%s/.libvirt/qemu", pw->pw_dir) == -1) { + qemudLog (QEMUD_ERR, "out of memory in asprintf"); + goto out_of_memory; + } + }
plus it's for a path names, use a MAX_PATH buffer, snprintf in it and the strdup() it. It won't be that much longuer and avoid the issue
[snip]
here too we are doing weird stuff to build paths, the erro code aren't checked a single snprintf to a MAX_PATH buffer followed by an strdup() would be cleaner IMHO. I understand it's not something added by the patch here, but probably woth fixing since similar to previous.
[...]
+ if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/qemu/log", pw->pw_dir) >= PATH_MAX) + goto snprintf_error; +
So server->logDir is a PATH_MAX array embbeded in the structure, I'm sure we don't need this and can allocate the string instead, no ?
I'll re-visit all these config file / log file path strings later, so we don't mix up plain re-factoring with other changes.
if (asprintf (&base, "%s/.libvirt/qemu", pw->pw_dir) == -1) { qemudLog (QEMUD_ERR, "out of memory in asprintf"); return -1; } }
one more here.
@@ -711,7 +691,6 @@ static struct qemud_server *qemudInitial }
/* We don't have a dom-0, so start from 1 */ - server->nextvmid = 1; server->sigread = sigread;
shouldn't the comment be removed too then ?
Yes - it should be moved to the new place where nextvmid is initialized. 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 -=|