On 01/10/2012 10:33 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx> > > To assist people in verifying that their host is operating in an > optimal manner, provide a 'virt-host-validate' command. For each > type of hypervisor, it will check any pre-requisites, or other > good recommendations and report what's working & what is not. > > eg > > # virt-host-validate > QEMU: Checking for device /dev/kvm : FAIL (Check that the 'kvm-intel' or 'kvm-amd' modules are loaded & the BIOS has enabled virtualization) > QEMU: Checking for device /dev/vhost : WARN (Load the 'vhost_net' module to improve performance of virtio networking) > QEMU: Checking for device /dev/net/tun : PASS > LXC: Checking for Linux >= 2.6.26 : PASS > > This warns people if they have vmx/svm, but don't have /dev/kvm. It > also warns about missing /dev/vhost net. > --- > tools/Makefile.am | 25 ++++++- > tools/virt-host-validate-common.c | 166 +++++++++++++++++++++++++++++++++++++ > tools/virt-host-validate-common.h | 53 ++++++++++++ > tools/virt-host-validate-lxc.c | 37 ++++++++ > tools/virt-host-validate-lxc.h | 27 ++++++ > tools/virt-host-validate-qemu.c | 50 +++++++++++ > tools/virt-host-validate-qemu.h | 27 ++++++ > tools/virt-host-validate.c | 77 +++++++++++++++++ > 8 files changed, 461 insertions(+), 1 deletions(-) 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). > @@ -64,6 +64,29 @@ virt-sanlock-cleanup: virt-sanlock-cleanup.in Makefile > virt-sanlock-cleanup.8: virt-sanlock-cleanup.in > $(AM_V_GEN)$(POD2MAN) $< $(srcdir)/$@ > > +virt_host_validate_SOURCES = \ > + virt-host-validate.c \ > + virt-host-validate-common.c virt-host-validate-common.h \ > + virt-host-validate-qemu.c virt-host-validate-qemu.h \ > + virt-host-validate-lxc.c virt-host-validate-lxc.h \ > + $(NULL) I know you are copying from existing patterns, but there was an interesting bug report raised recently where the use of backslash-newline followed by a macro that expands to the empty string triggers a bug in older bash: https://lists.gnu.org/archive/html/bug-automake/2012-01/msg00046.html I don't think it matters in our situation (particularly since we are feeding these macros into longer command lines, so that the overall command isn't ending in a backslash), but thought I'd mention it. > + > +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... > + ../src/libvirt.la \ > + ../gnulib/lib/libgnu.la \ > + $(NULL) > + > +virt_host_validate_CFLAGS = \ > + $(WARN_CFLAGS) \ you have them also listed here, in the correct location. > +++ b/tools/virt-host-validate-common.c > @@ -0,0 +1,166 @@ > +/* > + * virt-host-validate-common.c: Sanity check helper APis > + * > + * Copyright (C) 2011 Red Hat, Inc. s/2011/2012/ > + > +void virHostMsgCheck(const char *prefix, > + const char *format, > + ...) > +{ > + va_list args; > + char *msg; > + > + va_start(args, format); > + if (virVasprintf(&msg, format, args) < 0) { > + perror("malloc"); > + abort(); A bit harsh, but I suppose it's reasonable since OOM is unlikely for a program so simple (we aren't doing arbitrary actions based on user input, so our memory usage should be reasonably bounded). > + } > + 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? > + 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, ...? > +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? > +int virHostValidateHasCPUFlag(const char *name) > +{ > + FILE *fp = fopen("/proc/cpuinfo", "r"); > + int ret = 0; > + > + if (!fp) > + return 0; > + > + do { > + char line[1024]; > + > + if (!fgets(line, sizeof(line), fp)) > + break; > + > + if (strstr(line, name)) { > + ret = 1; > + break; > + } > + } while (1); > + > + fclose(fp); Doesn't 'make syntax-check' force us to use VIR_[FORCE_]FCLOSE here? > +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? > + > + if (sscanf(uts.release, "%d.%d.%d", &major, &minor, µ) != 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? > +++ b/tools/virt-host-validate-common.h > @@ -0,0 +1,53 @@ > +/* > + * virt-host-validate-common.h: Sanity check helper APis > + * > + * Copyright (C) 2011 Red Hat, Inc. s/2011/2012/, throughout the rest of the patch. > +#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. > + > +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. > + 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 like the overall idea, but there are probably some things to fix first. -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list