[PATCH libvirt v1] conf: verify for duplicate hostdevs

[Michal Prívozník]

On 6/18/21 12:46 PM, Shalini Chellathurai Saroja wrote:
> It is possible to define/edit(in shut off state) a domain XML with
> same hostdev device repeated more than once, as shown below. This
> behavior is not expected. So, this patch fixes it.
> 
> vser1:
> <domain type='kvm'>
> [...]
>   <devices>
>  [...]
>     <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-ccw'>
>       <source>
>         <address uuid='8e782fea-e5f4-45fa-a0f9-024cf66e5009'/>
>       </source>
>       <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0005'/>
>     </hostdev>
>     <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-ccw'>
>       <source>
>         <address uuid='8e782fea-e5f4-45fa-a0f9-024cf66e5009'/>
>       </source>
>       <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0006'/>
>     </hostdev>
> [...]
>   </devices>
> </domain>
> 
> $ virsh define vser1
> Domain 'vser1' defined from vser1
> 
> Signed-off-by: Shalini Chellathurai Saroja <shalini at linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
> ---
>  src/conf/domain_conf.c                        |  2 +-
>  src/conf/domain_conf.h                        |  2 +
>  src/conf/domain_validate.c                    | 21 ++++++++++
>  src/libvirt_private.syms                      |  1 +
>  .../hostdev-mdev-duplicate.err                |  1 +
>  .../hostdev-mdev-duplicate.xml                | 41 +++++++++++++++++++
>  .../hostdev-pci-duplicate.err                 |  1 +
>  .../hostdev-pci-duplicate.xml                 | 40 ++++++++++++++++++
>  .../hostdev-scsi-duplicate.err                |  1 +
>  .../hostdev-scsi-duplicate.xml                | 40 ++++++++++++++++++
>  .../hostdev-usb-duplicate.err                 |  1 +
>  .../hostdev-usb-duplicate.xml                 | 40 ++++++++++++++++++
>  tests/qemuxml2argvtest.c                      |  8 ++++
>  13 files changed, 198 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-duplicate.err
>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-pci-duplicate.err
>  create mode 100644 tests/qemuxml2argvdata/hostdev-pci-duplicate.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-duplicate.err
>  create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-duplicate.xml
>  create mode 100644 tests/qemuxml2argvdata/hostdev-usb-duplicate.err
>  create mode 100644 tests/qemuxml2argvdata/hostdev-usb-duplicate.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f65509d8..5746f69e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15369,7 +15369,7 @@ virDomainHostdevMatchCaps(virDomainHostdevDef *a,
>  }
>  
>  
> -static int
> +int
>  virDomainHostdevMatch(virDomainHostdevDef *a,
>                        virDomainHostdevDef *b)
>  {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f706c498..4d9d499b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3590,6 +3590,8 @@ virDomainHostdevDef *
>  virDomainHostdevRemove(virDomainDef *def, size_t i);
>  int virDomainHostdevFind(virDomainDef *def, virDomainHostdevDef *match,
>                           virDomainHostdevDef **found);
> +int virDomainHostdevMatch(virDomainHostdevDef *a,
> +                          virDomainHostdevDef *b);
>  
>  virDomainGraphicsListenDef *
>  virDomainGraphicsGetListen(virDomainGraphicsDef *def, size_t i);
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 2124d25d..df2ab473 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -1068,7 +1068,25 @@ virDomainDefDuplicateDiskInfoValidate(const virDomainDef *def)
>      return 0;
>  }
>  
> +static int
> +virDomainDefDuplicateHostdevInfoValidate(const virDomainDef *def)
> +{
> +    size_t i;
> +    size_t j;
>  
> +    for (i = 0; i < def->nhostdevs; i++) {
> +        for (j = i + 1; j < def->nhostdevs; j++) {
> +            if (virDomainHostdevMatch(def->hostdevs[i],
> +                                      def->hostdevs[j])) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                    _("Hostdev already exists in the domain configuration"));
> +                return -1;
> +            }
> +        }
> +    }
> +
> +    return 0;

So this forbids duplicated hostdevs. While there are some devices that
definitely can not be assigned twice (e.g. PCI devices), I'm not so sure
about some other types (e.g. iSCSI or scsi-host). But one can also argue
that such use case is broken. So let's do the following, I'll merge it
tomorrow, after the release so that we give users the longest window
possible to complain.

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

Michal