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

2018-03-06 Thread Michal Privoznik
On 03/02/2018 03:12 PM, John Ferlan wrote:
> 
> 
> 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 
>> ---
>>  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  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 
> 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.

The qemu-pr-helper binary is supposed to be suid. Just like the
bridge-helper. You know you need to uncomment this whenever you want to
change the default (displayed in the comment). Just like anything else
in the config file.
The whole idea of PR is so that your qemu can run unprivileged, without
rawio and all privileged operations are offloaded to the small, well
understood pr-helper.

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

Maybe for the managed case we can use -u $user -g $group so that
ph-helper changes UID/GID after it starts to that matching qemu process.
But I don't think it is strictly needed now. It would be nice to have
feature. Certainly not something we need to configure through config file.

Michal

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


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

2018-03-02 Thread 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 
> ---
>  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  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 
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