Re: [PATCH libvirt v1] conf: verify for duplicate hostdevs
On 7/1/21 4:40 PM, Michal Prívozník wrote: 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 ok, thank you Michal. Pushed now. Michal Thank you 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
Re: [PATCH libvirt v1] conf: verify for duplicate hostdevs
On 7/1/21 10:31 AM, Shalini Chellathurai Saroja wrote: > > 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: >>> >>> [...] >>> >>> [...] >>> >> model='vfio-ccw'> >>> >>> >>> >>> >>> >>> >> model='vfio-ccw'> >>> >>> >>> >>> >>> >>> [...] >>> >>> >>> >>> $ virsh define vser1 >>> Domain 'vser1' defined from vser1 >>> >>> Signed-off-by: Shalini Chellathurai Saroja >>> Reviewed-by: Bjoern Walk >>> Reviewed-by: Boris Fiuczynski >>> --- >>> 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 >> 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 > ok, thank you Michal. Pushed now. Michal
Re: [PATCH libvirt v1] conf: verify for duplicate hostdevs
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: [...] [...] [...] $ virsh define vser1 Domain 'vser1' defined from vser1 Signed-off-by: Shalini Chellathurai Saroja Reviewed-by: Bjoern Walk Reviewed-by: Boris Fiuczynski --- 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 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
Re: [PATCH libvirt v1] conf: verify for duplicate hostdevs
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: > > [...] > > [...] > > > > > > > > > > > > > [...] > > > > $ virsh define vser1 > Domain 'vser1' defined from vser1 > > Signed-off-by: Shalini Chellathurai Saroja > Reviewed-by: Bjoern Walk > Reviewed-by: Boris Fiuczynski > --- > 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 Michal
Re: [PATCH libvirt v1] conf: verify for duplicate hostdevs
ping 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: [...] [...] [...] $ virsh define vser1 Domain 'vser1' defined from vser1 Signed-off-by: Shalini Chellathurai Saroja Reviewed-by: Bjoern Walk Reviewed-by: Boris Fiuczynski --- 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; +} /** * virDomainDefDuplicateDriveAddressesValidate: @@ -1529,6 +1547,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefDuplicateDiskInfoValidate(def) < 0) return -1; +if (virDomainDefDuplicateHostdevInfoValidate(def) < 0) +return -1; + if (virDomainDefDuplicateDriveAddressesValidate(def) < 0) return -1; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2efa7876..f0d1b7b4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -457,6 +457,7 @@ virDomainHostdevDefFree; virDomainHostdevDefNew; virDomainHostdevFind; virDomainHostdevInsert; +virDomainHostdevMatch; virDomainHostdevModeTypeToString; virDomainHostdevRemove; virDomainHostdevSubsysPCIBackendTypeToString; diff --git a/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err new file mode 100644 index ..590afd30 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err @@ -0,0 +1 @@ +XML error: Hostdev already exists in the domain configuration diff --git a/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml new file mode 100644 index ..1c5e3263 ---
[PATCH libvirt v1] conf: verify for duplicate hostdevs
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: [...] [...] [...] $ virsh define vser1 Domain 'vser1' defined from vser1 Signed-off-by: Shalini Chellathurai Saroja Reviewed-by: Bjoern Walk Reviewed-by: Boris Fiuczynski --- 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; +} /** * virDomainDefDuplicateDriveAddressesValidate: @@ -1529,6 +1547,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefDuplicateDiskInfoValidate(def) < 0) return -1; +if (virDomainDefDuplicateHostdevInfoValidate(def) < 0) +return -1; + if (virDomainDefDuplicateDriveAddressesValidate(def) < 0) return -1; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2efa7876..f0d1b7b4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -457,6 +457,7 @@ virDomainHostdevDefFree; virDomainHostdevDefNew; virDomainHostdevFind; virDomainHostdevInsert; +virDomainHostdevMatch; virDomainHostdevModeTypeToString; virDomainHostdevRemove; virDomainHostdevSubsysPCIBackendTypeToString; diff --git a/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err new file mode 100644 index ..590afd30 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err @@ -0,0 +1 @@ +XML error: Hostdev already exists in the domain configuration diff --git a/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml new file mode 100644 index ..1c5e3263 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml @@ -0,0 +1,41 @@ + + QEMUGuest2 + c7a5fdbd-edaf-9455-926a-d65c16db1809 +