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

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



On 01/27/2012 07:59 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> 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.
> 
> In version 2:
> 
>   - Add to RPM specs
>   - Add man page
>   - Use getopt to parse --help, --version & --quiet options
>   - Don't use VT100 escape codes unless on a tty
>   - Disable QEMU / LXC checks if not compiled into libvirt

Looks like you hit my v1 review comments.

> +++ b/tools/virt-host-validate-common.c
> @@ -0,0 +1,214 @@
> +/*
> + * virt-host-validate-common.c: Sanity check helper APis

s/APis/APIs/

> +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);

This could have different failures, depending on whether it is called as
root or as an ordinary user; should we be trying to refine things if
/dev/kvm exists with 600 permissions but the current euid can't
read/write it?

> +int virHostValidateHasCPUFlag(const char *name)
> +{
> +    FILE *fp = fopen("/proc/cpuinfo", "r");
> +    int ret = 0;

You're using this like a bool, so maybe s/int/bool/ and s/0/false/ make
sense.

> +
> +    if (virParseVersionString(uts.release, &thisversion, true) < 0) {
> +        virHostMsgFail(level, hint);
> +        return -1;
> +    }
> +
> +    micro = (thisversion & 0xff);
> +    minor = ((thisversion >> 8) & 0xff);
> +    major = ((thisversion >> 16) & 0xff);
> +
> +    if (major > ((version >> 16) & 0xff)) {
> +        virHostMsgPass();
> +        return 0;
> +    } else if (major < ((version >> 16) & 0xff)) {
> +        virHostMsgFail(level, hint);
> +        return -1;
> +    }

Rather than break things down and check major/minor/micro independently,
why not just check if thisversion >= version and get all three checks
done at once?

> +++ b/tools/virt-host-validate-common.h
> @@ -0,0 +1,57 @@
> +/*
> + * virt-host-validate-common.h: Sanity check helper APis

s/APis/APIs/ (copy-paste strikes again :)

> +
> +int virHostValidateQEMU(void)
> +{
> +    int ret = 0;
> +
> +    if (virHostValidateHasCPUFlag("svm") ||
> +        virHostValidateHasCPUFlag("vmx")) {
> +        if (virHostValidateDevice("QEMU", "/dev/kvm",
> +                                  VIR_HOST_VALIDATE_FAIL,
> +                                  _("Check that the 'kvm-intel' or 'kvm-amd' modules are "
> +                                    "loaded & the BIOS has enabled virtualization")) < 0)
> +            ret = -1;
> +    }

Should we have an else clause with VIR_HOST_VALIDATE_WARN that hardware
lacks virtualization, therefore guests will run slower, when this is run
on older cpus?

> +
> +    if (virHostValidateDevice("QEMU", "/dev/vhost-net",
> +                              VIR_HOST_VALIDATE_WARN,
> +                              _("Load the 'vhost_net' module to improve performance "
> +                                "of virtio networking")) < 0)
> +        ret = -1;
> +
> +    if (virHostValidateDevice("QEMU", "/dev/net/tun",
> +                              VIR_HOST_VALIDATE_FAIL,
> +                              _("Load the 'tun' module to enable networking for QEMU guests")) < 0)
> +        ret = -1;
> +
> +    return ret;
> +}

Cool idea for another qemu validation - if !HAVE_YAJL, but qemu version
is 0.15 or newer, then complain that the libvirt binary lacks support
for QMP.

> +    while ((c = getopt_long(argc, argv, "hvq", argOptions, NULL)) != -1) {
> +        switch (c) {
> +        case 'v':
> +            show_version(stdout, argv[0]);
> +            return EXIT_SUCCESS;

Technically, we should still check for write errors to stdout, but
that's a bigger patch (gnulib provides a close-stdout module that lets
you add an atexit() hook to automate things, but it is not LGPLv2+), and
I'm okay if we overlook it for now.

> +
> +#if WITH_QEMU
> +    if ((!hvname || STREQ(hvname, "qemu")) &&
> +        virHostValidateQEMU() < 0)
> +        ret = EXIT_FAILURE;
> +#endif

Needs:

#else
    if (STREQ(hvname, "qemu"))
        fail; this libvirt was not compiled with qemu support

> +
> +#if WITH_LXC
> +    if ((!hvname || STREQ(hvname, "lxc")) &&
> +        virHostValidateLXC() < 0)
> +        ret = EXIT_FAILURE;
> +#endif

A similar #else complaining about no lxc support.

ACK - there are a bigger ideas for additional improvements that should
be followup patches, but what you have is reasonable for committing now
once you fix the minor typos.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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