[libvirt] [PATCH RFC] Add a virt-host-validate command to sanity check HV config

[Daniel P. Berrange]

On Tue, Jan 10, 2012 at 02:48:18PM -0700, Eric Blake wrote:
> On 01/10/2012 10:33 AM, Daniel P. Berrange wrote:
> 
> Missing changes to libvirt.spec.in to actually install it as part of the
> appropriate rpm (and probably to mingw32-libvirt.spec.in to exclude it,
> since neither kvm nor LXC is supported natively on mingw, leaving
> nothing to check for).

Actually I think it is reasonable to include the command for Mingw32.
I made it conditionally include the QEMU & LXC checks. Now it will
technically be a no-op on Win32 now, but someone could add checks for
existance of virtualbox perhaps.

> > +
> > +virt_host_validate_LDFLAGS = \
> > +		$(WARN_LDFLAGS) \
> > +		$(COVERAGE_LDFLAGS) \
> > +		$(NULL)
> > +
> > +virt_host_validate_LDADD = \
> > +		$(WARN_CFLAGS)					\
> 
> We shouldn't need $(WARN_CFLAGS) for LDADD, especially since...

Yes, that's a bug.

> > +    }
> > +    va_end(args);
> > +
> > +    fprintf(stdout, "%6s: Checking %-60s: ", prefix, msg);
> 
> Do we want to provide translation of these messages?  Or are you okay
> with hard-coding it to the English language, regardless of locale?

Yes, I added i18n to the updated version

> > +    VIR_FREE(msg);
> > +}
> > +
> > +void virHostMsgPass(void)
> > +{
> > +    fprintf(stdout, "\033[32mPASS\033[0m\n");
> 
> Are we sure that these particular terminal escape sequences will work
> everywhere, or should we be making things conditional?  And if
> conditional, what do we depend on? checking isatty(1), linking against
> ncurses, ...?

I made it conditional on isatty() in v2

> 
> > +int virHostValidateDevice(const char *hvname,
> > +                          const char *devname,
> > +                          virHostValidateLevel level,
> > +                          const char *hint)
> > +{
> > +    virHostMsgCheck(hvname, "for device %s", devname);
> > +
> > +    if (access(devname, R_OK|W_OK) < 0) {
> > +        virHostMsgFail(level, hint);
> > +        return -1;
> > +    }
> 
> Is this going to cause us grief if virt-host-validate is run as ordinary
> users instead of root?

Actually it is fine - /dev/kvm should be accessible & you'll get a
note if it isn't. For other things it will at least alert users that
if using libvirt non-root, they'll lack certain features

> > +    fclose(fp);
> 
> Doesn't 'make syntax-check' force us to use VIR_[FORCE_]FCLOSE here?

Yes it did :-)

> > +int virHostValidateLinuxKernel(const char *hvname,
> > +                               int version,
> > +                               virHostValidateLevel level,
> > +                               const char *hint)
> > +{
> 
> It sounds like this is Linux-only.  Should we be conditionally compiling
> things so that this helper app is only built and installed on Linux?

I don't think the helper needs to be conditional, since uname() is a
standard API call.

> > +
> > +    if (sscanf(uts.release, "%d.%d.%d", &major, &minor, &micro) != 3) {
> > +        micro = 0;
> > +        if (sscanf(uts.release, "%d.%d", &major, &minor) != 2) {
> > +            virHostMsgFail(level, hint);
> > +            return -1;
> > +        }
> > +    }
> 
> Rather than hand-parse things, can't you just use util.c's
> virParseVersionString?

Yes, that works too

> > +#include <config.h>
> > +
> > +#include "virt-host-validate-lxc.h"
> > +#include "virt-host-validate-common.h"
> > +
> > +int virHostValidateLXC(void)
> 
> Should this file (and virt-host-validate-qemu) only be compiled when
> their respective hypervisor drivers are also being compiled?  That is,
> you can build libvirtd with qemu but no lxc support, in which case, this
> helper app checking for lxc situations seems odd.

I have now made it conditional

> > +
> > +static void
> > +show_help(FILE *out, const char *argv0)
> > +{
> > +    fprintf(out, "syntax: %s [OPTIONS] [HVTYPE]\n", argv0);
> 
> What options?  I don't see any support for --help or --version.

There are some in v2 :-)

> > +    if (!textdomain(PACKAGE)) {
> > +        perror("textdomain");
> > +        return EXIT_FAILURE;
> > +    }
> > +
> > +    if (argc > 2) {
> > +        fprintf(stderr, "too many command line arguments\n");
> > +        show_help(stderr, argv[0]);
> > +        return EXIT_FAILURE;
> > +    }
> 
> Should we be parsing options before this argc > 2 check?

I use getopt_long() now


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|