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

[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 <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.

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