[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] [RFC] 0/3 Add Linux Container support to libvirt

Dave Leskovec <dlesko linux vnet ibm com> 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:
   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) {


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]