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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list