[PATCH 2/2] qemu: Do not erase input device from namespace if duplicate

[Michal Prívozník]

On 7/7/21 12:46 PM, Kristina Hanicova wrote:
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1780508
> 
> Signed-off-by: Kristina Hanicova <khanicov at redhat.com>
> ---

I know writing commit messages is hard, but I think you can describe the
problem and the proposed solution briefly.

>  src/qemu/qemu_hotplug.c   |  3 +--
>  src/qemu/qemu_namespace.c | 10 ++++++++--
>  src/qemu/qemu_namespace.h |  3 ++-
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index d2a354d026..7002623924 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3225,9 +3225,8 @@ qemuDomainAttachInputDevice(virQEMUDriver *driver,
>      if (qemuBuildInputDevStr(&devstr, vm->def, input, priv->qemuCaps) < 0)
>          goto cleanup;
>  
> -    if (qemuDomainNamespaceSetupInput(vm, input) < 0)
> +    if (qemuDomainNamespaceSetupInput(vm, input, &teardowndevice) < 0)
>          goto cleanup;
> -    teardowndevice = true;
>  
>      if (qemuSetupInputCgroup(vm, input) < 0)
>          goto cleanup;
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index 154968acbd..22bdda229a 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -1600,9 +1600,11 @@ qemuDomainNamespaceTeardownRNG(virDomainObj *vm,
>  
>  int
>  qemuDomainNamespaceSetupInput(virDomainObj *vm,
> -                              virDomainInputDef *input)
> +                              virDomainInputDef *input,
> +                              bool *created)
>  {
>      g_autoptr(virGSListString) paths = NULL;
> +    int ret = 0;
>  
>      if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>          return 0;
> @@ -1610,8 +1612,12 @@ qemuDomainNamespaceSetupInput(virDomainObj *vm,
>      if (qemuDomainSetupInput(input, &paths) < 0)
>          return -1;
>  
> -    if (qemuNamespaceMknodPaths(vm, paths) < 0)
> +    if ((ret = qemuNamespaceMknodPaths(vm, paths)) < 0)
>          return -1;
> +
> +    if (ret == 0)
> +        *created = true;
> +
>      return 0;
>  }
>  
> diff --git a/src/qemu/qemu_namespace.h b/src/qemu/qemu_namespace.h
> index 771d7873ef..5d9af123a9 100644
> --- a/src/qemu/qemu_namespace.h
> +++ b/src/qemu/qemu_namespace.h
> @@ -80,7 +80,8 @@ int qemuDomainNamespaceTeardownRNG(virDomainObj *vm,
>                                     virDomainRNGDef *rng);
>  
>  int qemuDomainNamespaceSetupInput(virDomainObj *vm,
> -                                  virDomainInputDef *input);
> +                                  virDomainInputDef *input,
> +                                  bool *created);
>  
>  int qemuDomainNamespaceTeardownInput(virDomainObj *vm,
>                                       virDomainInputDef *input);
> 

So this fixes just <input/>. I know that we talked about other types and
that QEMU can open them multiple times, but I still thought that you
were going to make the rest of qemuDomainNamespaceSetup*() follow this
Input(). I mean, even though we did not see it in our testing, but the
hotplug code can still experience an error (for various reasons) in
which case we shouldn't try to remove the device from the namespace.

For instance: when hotplugging an USB device
qemuDomainAttachHostUSBDevice() is called and if say
qemuSetupHostdevCgroup() failed then the device would be removed from
namespace even though we might not have created it.

Having said that, I wonder if we can save a few lines of code if this
@created argument was set in qemuNamespaceMknodPaths() instead of having
the same pattern (if ret==0 then *created=true;) copied into all
qemuDomainNamespaceSetup*() functions. But I don't mind.

Michal