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:

> +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?

> + > +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 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.