Dave Leskovec <dlesko@xxxxxxxxxxxxxxxxxx> wrote:
> The following set of patches add the first batch of linux container
> support to libvirt. The work is not complete but I wanted to start
> getting some of this out for comments. This set of patches supports
> the following:
Hi Dave,
Something (your mail client?) seems to have corrupted those patches
by removing leading spaces and splitting lines, so most of the parts
do not apply:
$ patch -p0 < /t/p
patching file configure.in
patch: **** malformed patch at line 30: [ --with-qemu add QEMU/KVM support (on)],[],[with_qemu=yes])
I know you said this is only preliminary,
so here are some mostly-superficial suggestions:
* please mark all new diagnostics with _(...), so that translators
see them. Since you add a new *Error function, please add its name to
the err_func_re definition in Makefile.maint. Then, running
"make syntax-check" will inform you of remaining unmarked strings.
Also, declare your new log function with the printf __attribute__,
so that gcc checks each format string against its arguments.
[Go ahead and add your new function for now, but I suspect there
should not be a new log function for each new subsystem.
These are all nearly-identical clones:
qemudReportError
virStorageReportError
ReportError
I hope someone finds the time to factor this out.
It's on my medium-term TODO list.
]
* check for strdup failure if the resulting pointer may be dereferenced
without being checked for NULL. There seem to be two of those:
one in lxcParseDomainName, the other in lxcLoadDriverConfig.
In each case, it looks like the pointer is later assumed to be
non-NULL and dereferenced (probable segfault).
* Something to think about:
People should be able to install libvirt using e.g., --prefix=/foo,
which would ideally make your code use "/foo/etc/libvirt/lxc",
rather than the currently-hard-coded "/etc/libvirt/lxc".
Or maybe that file name should be even more configurable...
Note that openvz_conf.c has a similar problem, though at least it
does work with --prefix=/usr/local.
* please use virBufferAddLit when there is no % directive:
$ grep -E 'virBufferVSprintf *\([^,]+, *"[^%]+"\)' /t/p
+ if (virBufferVSprintf(buf, " <container>\n") < 0) {
+ if (virBufferVSprintf(buf, " </filesystem>\n") < 0) {
+ if (virBufferVSprintf(buf, " </container>\n") < 0) {
+ if (virBufferVSprintf(buf, " <devices>\n") < 0) {
Jim
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list