Re: [PATCH libvirt v1] conf: verify for duplicate hostdevs

[Shalini Chellathurai Saroja <shalini@xxxxxxxxxxxxx>]

On 6/30/21 4:49 PM, Michal Prívozník wrote:
> 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@xxxxxxxxxxxxx>
> > Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
> > Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
ok, thank you Michal.
> Michal
--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294