[libvirt] [PATCH RFC] Add a virt-host-validate command to sanity check HV config
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. --- 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(-) create mode 100644 tools/virt-host-validate-common.c create mode 100644 tools/virt-host-validate-common.h create mode 100644 tools/virt-host-validate-lxc.c create mode 100644 tools/virt-host-validate-lxc.h create mode 100644 tools/virt-host-validate-qemu.c create mode 100644 tools/virt-host-validate-qemu.h create mode 100644 tools/virt-host-validate.c diff --git a/tools/Makefile.am b/tools/Makefile.am index 6705546..2b14319 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -30,7 +30,7 @@ EXTRA_DIST = \ DISTCLEANFILES = bin_SCRIPTS = virt-xml-validate virt-pki-validate -bin_PROGRAMS = virsh +bin_PROGRAMS = virsh virt-host-validate if HAVE_SANLOCK sbin_SCRIPTS = virt-sanlock-cleanup @@ -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) + +virt_host_validate_LDFLAGS = \ + $(WARN_LDFLAGS) \ + $(COVERAGE_LDFLAGS) \ + $(NULL) + +virt_host_validate_LDADD = \ + $(WARN_CFLAGS) \ + ../src/libvirt.la \ + ../gnulib/lib/libgnu.la \ + $(NULL) + +virt_host_validate_CFLAGS = \ + $(WARN_CFLAGS) \ + $(COVERAGE_CFLAGS) \ + $(NULL) + virsh_SOURCES = \ console.c console.h \ virsh.c diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c new file mode 100644 index 0000000..7ada218 --- /dev/null +++ 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. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <config.h> + +#include <stdarg.h> +#include <stdlib.h> +#include <stdio.h> +#include <unistd.h> +#include <sys/utsname.h> + +#include "util.h" +#include "memory.h" +#include "virt-host-validate-common.h" + +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(); + } + va_end(args); + + fprintf(stdout, "%6s: Checking %-60s: ", prefix, msg); + VIR_FREE(msg); +} + +void virHostMsgPass(void) +{ + fprintf(stdout, "\033[32mPASS\033[0m\n"); +} + +void virHostMsgFail(virHostValidateLevel level, + const char *hint) +{ + if (level == VIR_HOST_VALIDATE_FAIL) + fprintf(stdout, "\033[31mFAIL\033[0m (%s)\n", hint); + else if (level == VIR_HOST_VALIDATE_WARN) + fprintf(stdout, "\033[33mWARN\033[0m (%s)\n", hint); + else if (level == VIR_HOST_VALIDATE_NOTE) + fprintf(stdout, "\033[34mNOTE\033[0m (%s)\n", hint); +} + + +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; + } + + virHostMsgPass(); + return 0; +} + + +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); + + return ret; +} + + +int virHostValidateLinuxKernel(const char *hvname, + int version, + virHostValidateLevel level, + const char *hint) +{ + struct utsname uts; + int major, minor, micro; + + uname(&uts); + + virHostMsgCheck(hvname, "for Linux >= %d.%d.%d", + ((version >> 16) & 0xff), + ((version >> 8) & 0xff), + (version & 0xff)); + + if (STRNEQ(uts.sysname, "Linux")) { + virHostMsgFail(level, hint); + return -1; + } + + 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; + } + } + + if (major > ((version >> 16) & 0xff)) { + virHostMsgPass(); + return 0; + } else if (major < ((version >> 16) & 0xff)) { + virHostMsgFail(level, hint); + return -1; + } + + if (minor > ((version >> 8) & 0xff)) { + virHostMsgPass(); + return 0; + } else if (minor < ((version >> 8) & 0xff)) { + virHostMsgFail(level, hint); + return -1; + } + + if (micro > (version & 0xff)) { + virHostMsgPass(); + return 0; + } else if (micro < (version & 0xff)) { + virHostMsgFail(level, hint); + return -1; + } + + virHostMsgPass(); + return 0; +} diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h new file mode 100644 index 0000000..6a4d1ac --- /dev/null +++ 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. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#ifndef __VIRT_HOST_VALIDATE_COMMON_H__ +#define __VIRT_HOST_VALIDATE_COMMON_H__ + +#include "internal.h" + +typedef enum { + VIR_HOST_VALIDATE_FAIL, + VIR_HOST_VALIDATE_WARN, + VIR_HOST_VALIDATE_NOTE, +} virHostValidateLevel; + +extern void virHostMsgCheck(const char *prefix, + const char *format, + ...) ATTRIBUTE_FMT_PRINTF(2, 3); + +extern void virHostMsgPass(void); +extern void virHostMsgFail(virHostValidateLevel level, + const char *hint); + +extern int virHostValidateDevice(const char *hvname, + const char *devname, + virHostValidateLevel level, + const char *hint); + +extern int virHostValidateHasCPUFlag(const char *name); + +extern int virHostValidateLinuxKernel(const char *hvname, + int version, + virHostValidateLevel level, + const char *hint); + +#endif /* __VIRT_HOST_VALIDATE_COMMON_H__ */ diff --git a/tools/virt-host-validate-lxc.c b/tools/virt-host-validate-lxc.c new file mode 100644 index 0000000..f41cf78 --- /dev/null +++ b/tools/virt-host-validate-lxc.c @@ -0,0 +1,37 @@ +/* + * virt-host-validate-lxc.c: Sanity check a LXC hypervisor host + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <config.h> + +#include "virt-host-validate-lxc.h" +#include "virt-host-validate-common.h" + +int virHostValidateLXC(void) +{ + int ret = 0; + + if (virHostValidateLinuxKernel("LXC", (2 << 16) | (6 << 8) | 26, + VIR_HOST_VALIDATE_FAIL, + "Upgrade to a kernel supporting namespaces") < 0) + ret = -1; + + return ret; +} diff --git a/tools/virt-host-validate-lxc.h b/tools/virt-host-validate-lxc.h new file mode 100644 index 0000000..3cae39f --- /dev/null +++ b/tools/virt-host-validate-lxc.h @@ -0,0 +1,27 @@ +/* + * virt-host-validate-lxc.h: Sanity check a LXC hypervisor host + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#ifndef __VIRT_HOST_VALIDATE_LXC_H__ +#define __VIRT_HOST_VALIDATE_LXC_H__ + +extern int virHostValidateLXC(void); + +#endif /* __VIRT_HOST_VALIDATE_LXC_H__ */ diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c new file mode 100644 index 0000000..b0e4fe5 --- /dev/null +++ b/tools/virt-host-validate-qemu.c @@ -0,0 +1,50 @@ +/* + * virt-host-validate-qemu.c: Sanity check a QEMU hypervisor host + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <config.h> + +#include "virt-host-validate-qemu.h" +#include "virt-host-validate-common.h" + +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; + } + + if (virHostValidateDevice("QEMU", "/dev/vhost", + 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; +} diff --git a/tools/virt-host-validate-qemu.h b/tools/virt-host-validate-qemu.h new file mode 100644 index 0000000..e1dbcbe --- /dev/null +++ b/tools/virt-host-validate-qemu.h @@ -0,0 +1,27 @@ +/* + * virt-host-validate-qemu.h: Sanity check a QEMU hypervisor host + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#ifndef __VIRT_HOST_VALIDATE_QEMU_H__ +#define __VIRT_HOST_VALIDATE_QEMU_H__ + +extern int virHostValidateQEMU(void); + +#endif /* __VIRT_HOST_VALIDATE_QEMU_H__ */ diff --git a/tools/virt-host-validate.c b/tools/virt-host-validate.c new file mode 100644 index 0000000..b94aa8f --- /dev/null +++ b/tools/virt-host-validate.c @@ -0,0 +1,77 @@ +/* + * virt-host-check.c: Sanity check a hypervisor host + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <gettext.h> + +#include "internal.h" +#include "configmake.h" + +#include "virt-host-validate-qemu.h" +#include "virt-host-validate-lxc.h" + +static void +show_help(FILE *out, const char *argv0) +{ + fprintf(out, "syntax: %s [OPTIONS] [HVTYPE]\n", argv0); +} + +int +main(int argc, char **argv) +{ + const char *hvname = NULL; + int ret = EXIT_SUCCESS; + + if (!setlocale(LC_ALL, "")) { + perror("setlocale"); + /* failure to setup locale is not fatal */ + } + if (!bindtextdomain(PACKAGE, LOCALEDIR)) { + perror("bindtextdomain"); + return EXIT_FAILURE; + } + 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; + } + + if (argc > 1) + hvname = argv[1]; + + if ((!hvname || STREQ(hvname, "qemu")) && + virHostValidateQEMU() < 0) + ret = EXIT_FAILURE; + + if ((!hvname || STREQ(hvname, "lxc")) && + virHostValidateLXC() < 0) + ret = EXIT_FAILURE; + + return ret; +} -- 1.7.7.5
On 01/10/2012 12:33 PM, 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.
In general this seems like a very useful idea, and this looks like a good start. I'm wondering how much networking-related stuff we can put in here. For example, lately it would have been useful to have a check looking for a system-wide dnsmasq instance listening on all interfaces. I also recall cases where iptables/ip6tables/radvd weren't installed properly/completely (this seems to usually happen on gentoo, due to its "build it yourself" nature). But of course that wouldn't signal a problem on a system that wasn't intending to have libvirt setup DHCP for the guests. (Oh, another check related to vhost - if the kernel/iptables version is "too low" (I would have to look up the exact versions), then vhost-net *shouldn't* be loaded if there are RHEL5 (and other older) guests using DHCP and getting addresses from a dhcp server on the host. But again, that's not *always* a problem.) (I've been thinking about some sort of program that could diagnose problems with the network plumbing of a guest (by looking at brctl/etc output, then doing a tcpdump of the vnetX, virbrY, and ethZ interfaces while attempting to ping in/out from the guest, and seeing how far the traffic went), but aside from the complexity created by needing to execute things on the guest, I anyway don't know how much concrete help that could give beyond saying "start looking here".)
On 01/10/2012 10:33 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. --- 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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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, µ) != 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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump