9:04 a.m.
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 :|