6:16 a.m.
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.
Not a blocker though, but that makes me feel uneasy. Let's use a real
function or a macro, it's not like we need to optimize at the tenth of micro
second in libvirt :-)
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.
[...]
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.
[...]
+ 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
+
+ /* Configuration paths are either ~/.libvirt/qemu/... (session) or
+ * /etc/libvirt/qemu/... (system).
+ */
+ if (asprintf (&qemu_driver->configDir, "%s", base) == -1)
+ goto out_of_memory;
+
+ if (asprintf (&qemu_driver->autostartDir, "%s/autostart", base) ==
-1)
+ goto out_of_memory;
+
+ if (asprintf (&qemu_driver->networkConfigDir, "%s/networks", base)
== -1)
+ goto out_of_memory;
+
+ if (asprintf (&qemu_driver->networkAutostartDir,
"%s/networks/autostart",
+ base) == -1)
+ goto out_of_memory;
[...]
@@ -499,25 +596,25 @@ int qemudStartVMDaemon(struct qemud_serv
} else
vm->def->vncActivePort = vm->def->vncPort;
- if ((strlen(server->logDir) + /* path */
+ if ((strlen(driver->logDir) + /* path */
1 + /* Separator */
strlen(vm->def->name) + /* basename */
4 + /* suffix .log */
1 /* NULL */) > PATH_MAX) {
qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
"config file path too long: %s/%s.log",
- server->logDir, vm->def->name);
- return -1;
- }
- strcpy(logfile, server->logDir);
+ driver->logDir, vm->def->name);
+ return -1;
+ }
+ strcpy(logfile, driver->logDir);
strcat(logfile, "/");
strcat(logfile, vm->def->name);
strcat(logfile, ".log");
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 ?
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 ?
I would say, still apply the patch but we must remember to clean up those
paths allocations. Maybe a single vararg function to build up paths and
return the new string would be a good separate cleanup, that we can probably
do at a later stage.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/