[libvirt] [PATCH v2 07/12] qemu: Introduce pr_helper to qemu.conf

[John Ferlan]

On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Just like we allow users overriding path to bridge-helper
> detected at compile time we can allow them to override path to
> qemu-pr-helper.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  m4/virt-driver-qemu.m4             | 5 +++++
>  src/qemu/libvirtd_qemu.aug         | 1 +
>  src/qemu/qemu.conf                 | 4 ++++
>  src/qemu/qemu_conf.c               | 7 ++++++-
>  src/qemu/qemu_conf.h               | 1 +
>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>  6 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4
> index b9bafdab9..0e0f261d6 100644
> --- a/m4/virt-driver-qemu.m4
> +++ b/m4/virt-driver-qemu.m4
> @@ -57,6 +57,11 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [
>                 [/usr/libexec:/usr/lib/qemu:/usr/lib])
>    AC_DEFINE_UNQUOTED([QEMU_BRIDGE_HELPER], ["$QEMU_BRIDGE_HELPER"],
>                       [QEMU bridge helper])
> +  AC_PATH_PROG([QEMU_PR_HELPER], [qemu-pr-helper],
> +               [/usr/libexec/qemu-pr-helper],
> +               [/usr/libexec:/usr/lib/qemu:/usr/lib])
> +  AC_DEFINE_UNQUOTED([QEMU_PR_HELPER], ["$QEMU_PR_HELPER"],
> +                     [QEMU PR helper])
>  ])
>  
>  AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index c19bf3a43..2dc16e91f 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -86,6 +86,7 @@ module Libvirtd_qemu =
>     let process_entry = str_entry "hugetlbfs_mount"
>                   | bool_entry "clear_emulator_capabilities"
>                   | str_entry "bridge_helper"
> +                 | str_entry "pr_helper"
>                   | bool_entry "set_process_name"
>                   | int_entry "max_processes"
>                   | int_entry "max_files"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 43dd561cc..4bc668406 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -775,3 +775,7 @@
>  # This directory is used for memoryBacking source if configured as file.
>  # NOTE: big files will be stored here
>  #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
> +
> +# Path to the SCSI persistent reservations helper. This helper is
> +# used whenever <reservations/> are enabled for SCSI disks.
> +#pr_helper = "/usr/libexec/qemu-pr-helper"

So "how" would I know as a user/admin whether or not I *needed* to
uncomment this? or how it's used. From how I read bridge_helper it's
there because "This executable is used to create <source type='bridge'>
interfaces when libvirtd is running unprivileged." - So then the
question becomes does PR make sense or is it useful for the running
unprivileged case? In fact, IIRC, using PR requires a bit of special
permissions and certain capabilities anyway (related to sg_io or rawio).
So is having this available in qemu.conf "required"? I cannot imagine
the name of the image changing.

Are there any other qualifiers than '-k' that may be useful to turn on
in the pr helper...

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 36cf3a281..8c69dbe75 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -307,7 +307,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>              goto error;
>      }
>  
> -    if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0)
> +    if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0 ||
> +        VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0)
>          goto error;
>  
>      cfg->clearEmulatorCapabilities = true;
> @@ -392,6 +393,7 @@ static void virQEMUDriverConfigDispose(void *obj)
>      }
>      VIR_FREE(cfg->hugetlbfs);
>      VIR_FREE(cfg->bridgeHelperName);
> +    VIR_FREE(cfg->prHelperName);
>  
>      VIR_FREE(cfg->saveImageFormat);
>      VIR_FREE(cfg->dumpImageFormat);
> @@ -759,6 +761,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>      if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0)
>          goto cleanup;
>  
> +    if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0)
> +        goto cleanup;
> +
>      if (virConfGetValueBool(conf, "mac_filter", &cfg->macFilter) < 0)
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 83fd45282..5baa3af5d 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -153,6 +153,7 @@ struct _virQEMUDriverConfig {
>      size_t nhugetlbfs;
>  
>      char *bridgeHelperName;
> +    char *prHelperName;
>  
>      bool macFilter;
>  
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
> index 688e5b9fd..c0efae47b 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -100,3 +100,4 @@ module Test_libvirtd_qemu =
>      { "1" = "mount" }
>  }
>  { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" }
> +{ "pr_helper" = "/usr/libexec/qemu-pr-helper" }
> 

I'm kind of surprised this passed the testing... I have this
recollection of these aug files being really particular about the order
and I would "expect" the "pr_helper" to be closer to the
"bridge_helper".  Ahh, I see, the placement in qemu.conf at the end is
what allows this to work as it is...

John