Re: [PATCH 2/4] virt-host-validate: distinguish exists vs accessible for devices

[Laine Stump <laine@xxxxxxxxx>]

On 10/07/2015 12:50 PM, Daniel P. Berrange wrote:
> Currently we just check that various devices are accessible.
> This leads to inaccurate errors reported for /dev/kvm and
> /dev/vhost-net if they exist but an unprivileged user lacks
> access. Switch existing checks to look for file existance,
> and add a separate check for accessibility of /dev/kvm
> since some distros don't grant users access by default.

One problem with this is that the people with those distros probably 
won't be running virt-host-validate under the same uid as used by 
libvirt when running qemu, so the results won't necessarily tell you 
what you need - if you run it as root it will say that /dev/kvm is 
accessible, even though it may not be for the case of the "qemu user", 
and if you run it as some unprivileged user, if may say that /dev/kvm 
*isn't* accessible, even though it is in the case of the qemu user.

(This came to mind because someone in #virt recently was trying to 
figure out why kvm wasn't working for them, showed that they had 
/dev/kvm with mode 700, and someone else pointed out that this is how 
it's shipped for [some other distro, I forget which].)

ACK other than that though.
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
>   tools/virt-host-validate-common.c | 27 ++++++++++++++++++++++-----
>   tools/virt-host-validate-common.h | 13 +++++++++----
>   tools/virt-host-validate-qemu.c   | 28 ++++++++++++++++------------
>   3 files changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
> index 12a98f4..d646a79 100644
> --- a/tools/virt-host-validate-common.c
> +++ b/tools/virt-host-validate-common.c
> @@ -115,12 +115,29 @@ void virHostMsgFail(virHostValidateLevel level,
>   }
>   
>   
> -int virHostValidateDevice(const char *hvname,
> -                          const char *dev_name,
> -                          virHostValidateLevel level,
> -                          const char *hint)
> +int virHostValidateDeviceExists(const char *hvname,
> +                                const char *dev_name,
> +                                virHostValidateLevel level,
> +                                const char *hint)
>   {
> -    virHostMsgCheck(hvname, "for device %s", dev_name);
> +    virHostMsgCheck(hvname, "if device %s exists", dev_name);
> +
> +    if (access(dev_name, F_OK) < 0) {
> +        virHostMsgFail(level, hint);
> +        return -1;
> +    }
> +
> +    virHostMsgPass();
> +    return 0;
> +}
> +
> +
> +int virHostValidateDeviceAccessible(const char *hvname,
> +                                    const char *dev_name,
> +                                    virHostValidateLevel level,
> +                                    const char *hint)
> +{
> +    virHostMsgCheck(hvname, "if device %s is accessible", dev_name);
>   
>       if (access(dev_name, R_OK|W_OK) < 0) {
>           virHostMsgFail(level, hint);
> diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h
> index 9d8bcea..c25e69c 100644
> --- a/tools/virt-host-validate-common.h
> +++ b/tools/virt-host-validate-common.h
> @@ -42,10 +42,15 @@ extern void virHostMsgPass(void);
>   extern void virHostMsgFail(virHostValidateLevel level,
>                              const char *hint);
>   
> -extern int virHostValidateDevice(const char *hvname,
> -                                 const char *dev_name,
> -                                 virHostValidateLevel level,
> -                                 const char *hint);
> +extern int virHostValidateDeviceExists(const char *hvname,
> +                                       const char *dev_name,
> +                                       virHostValidateLevel level,
> +                                       const char *hint);
> +
> +extern int virHostValidateDeviceAccessible(const char *hvname,
> +                                           const char *dev_name,
> +                                           virHostValidateLevel level,
> +                                           const char *hint);
>   
>   extern bool virHostValidateHasCPUFlag(const char *name);
>   
> diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c
> index 33a5738..9486064 100644
> --- a/tools/virt-host-validate-qemu.c
> +++ b/tools/virt-host-validate-qemu.c
> @@ -20,7 +20,6 @@
>    */
>   
>   #include <config.h>
> -
>   #include "virt-host-validate-qemu.h"
>   #include "virt-host-validate-common.h"
>   
> @@ -32,25 +31,30 @@ int virHostValidateQEMU(void)
>       if (virHostValidateHasCPUFlag("svm") ||
>           virHostValidateHasCPUFlag("vmx")) {
>           virHostMsgPass();
> -        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)
> +        if (virHostValidateDeviceExists("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;
> +        else if (virHostValidateDeviceAccessible("QEMU", "/dev/kvm",
> +                                                 VIR_HOST_VALIDATE_FAIL,
> +                                                 _("Check /dev/kvm is world writable or you are in "
> +                                                   "a group that is allowed to access it")) < 0)
>               ret = -1;
>       } else {
>           virHostMsgFail(VIR_HOST_VALIDATE_WARN,
>                          _("Only emulated CPUs are available, performance will be significantly limited"));
>       }
>   
> -    if (virHostValidateDevice("QEMU", "/dev/vhost-net",
> -                              VIR_HOST_VALIDATE_WARN,
> -                              _("Load the 'vhost_net' module to improve performance "
> -                                "of virtio networking")) < 0)
> +    if (virHostValidateDeviceExists("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)
> +    if (virHostValidateDeviceExists("QEMU", "/dev/net/tun",
> +                                    VIR_HOST_VALIDATE_FAIL,
> +                                    _("Load the 'tun' module to enable networking for QEMU guests")) < 0)
>           ret = -1;
>   
>       return ret;

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list