Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On 11/13/18 9:27 AM, Erik Skultety wrote: On Mon, Nov 12, 2018 at 04:28:09PM +0100, Boris Fiuczynski wrote: On 11/12/18 1:14 PM, Erik Skultety wrote: Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner. Signed-off-by: Erik Skultety --- src/qemu/qemu_domain.c | 35 +-- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _(" attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { +if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, } +static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + Syntax-check does not like the above blank line... :-) Wow, good catch, quite interesting though, because I always build my patches on a bunch of different distros (a reasonable subset of those in our CI) and none of them reported this. What's your setup if may ask? Fedora 28 on s390 running in a VPATH build make check followed by make syntax-check +switch ((virMediatedDeviceModelType) mdevsrc->model) { +case VIR_MDEV_MODEL_TYPE_VFIO_PCI: +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); +case VIR_MDEV_MODEL_TYPE_VFIO_AP: +break; +case VIR_MDEV_MODEL_TYPE_VFIO_CCW: +break; +case VIR_MDEV_MODEL_TYPE_LAST: +virReportEnumRangeError(virMediatedDeviceModelType, +mdevsrc->model); +return -1; +} + +return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def, Besides Marc question regard the default switch case Reviewed-by: Boris Fiuczynski Thanks, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On Mon, Nov 12, 2018 at 04:28:09PM +0100, Boris Fiuczynski wrote: > On 11/12/18 1:14 PM, Erik Skultety wrote: > > Since we'll need to validate other models apart from VFIO PCI too, > > having a helper for each model should keep the code base cleaner. > > > > Signed-off-by: Erik Skultety > > --- > > src/qemu/qemu_domain.c | 35 +-- > > 1 file changed, 29 insertions(+), 6 deletions(-) > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > index e25afdad6b..17d207513d 100644 > > --- a/src/qemu/qemu_domain.c > > +++ b/src/qemu/qemu_domain.c > > @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const > > virDomainNetDef *net) > > static int > > -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, > > - const virDomainDef *def, > > - virQEMUCapsPtr qemuCaps) > > +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev > > *dev, > > + const virDomainDef *def, > > + virQEMUCapsPtr qemuCaps) > > { > > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) > > +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) > > return 0; > > if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { > > @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const > > virDomainHostdevSubsysMediatedDev *mdevsrc, > > return -1; > > } > > -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { > > +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { > > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > _(" attribute 'display' is only supported" > >" with model='vfio-pci'")); > > @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const > > virDomainHostdevSubsysMediatedDev *mdevsrc, > > return -1; > > } > > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { > > +if (dev->display == VIR_TRISTATE_SWITCH_ON) { > > if (def->ngraphics == 0) { > > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > _("graphics device is needed for attribute > > value " > > @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const > > virDomainHostdevSubsysMediatedDev *mdevsrc, > > } > > +static int > > +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, > > + const virDomainDef *def, > > + virQEMUCapsPtr qemuCaps) > > +{ > > + > Syntax-check does not like the above blank line... :-) Wow, good catch, quite interesting though, because I always build my patches on a bunch of different distros (a reasonable subset of those in our CI) and none of them reported this. What's your setup if may ask? > > > +switch ((virMediatedDeviceModelType) mdevsrc->model) { > > +case VIR_MDEV_MODEL_TYPE_VFIO_PCI: > > +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); > > +case VIR_MDEV_MODEL_TYPE_VFIO_AP: > > +break; > > +case VIR_MDEV_MODEL_TYPE_VFIO_CCW: > > +break; > > +case VIR_MDEV_MODEL_TYPE_LAST: > > +virReportEnumRangeError(virMediatedDeviceModelType, > > +mdevsrc->model); > > +return -1; > > +} > > + > > +return 0; > > +} > > + > > + > > static int > > qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, > > const virDomainDef *def, > > > Besides Marc question regard the default switch case > Reviewed-by: Boris Fiuczynski Thanks, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On 11/12/18 1:14 PM, Erik Skultety wrote: Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner. Signed-off-by: Erik Skultety --- src/qemu/qemu_domain.c | 35 +-- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _(" attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { +if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, } +static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + Syntax-check does not like the above blank line... :-) +switch ((virMediatedDeviceModelType) mdevsrc->model) { +case VIR_MDEV_MODEL_TYPE_VFIO_PCI: +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); +case VIR_MDEV_MODEL_TYPE_VFIO_AP: +break; +case VIR_MDEV_MODEL_TYPE_VFIO_CCW: +break; +case VIR_MDEV_MODEL_TYPE_LAST: +virReportEnumRangeError(virMediatedDeviceModelType, +mdevsrc->model); +return -1; +} + +return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def, Besides Marc question regard the default switch case Reviewed-by: Boris Fiuczynski -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On Mon, Nov 12, 2018 at 03:03:34PM +0100, Marc Hartmayer wrote: > On Mon, Nov 12, 2018 at 01:14 PM +0100, Erik Skultety > wrote: > > Since we'll need to validate other models apart from VFIO PCI too, > > having a helper for each model should keep the code base cleaner. > > > > Signed-off-by: Erik Skultety > > --- > > src/qemu/qemu_domain.c | 35 +-- > > 1 file changed, 29 insertions(+), 6 deletions(-) > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > index e25afdad6b..17d207513d 100644 > > --- a/src/qemu/qemu_domain.c > > +++ b/src/qemu/qemu_domain.c > > @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const > > virDomainNetDef *net) > > > > > > static int > > -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, > > - const virDomainDef *def, > > - virQEMUCapsPtr qemuCaps) > > +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev > > *dev, > > + const virDomainDef *def, > > + virQEMUCapsPtr qemuCaps) > > { > > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) > > +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) > > return 0; > > > > if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { > > @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const > > virDomainHostdevSubsysMediatedDev *mdevsrc, > > return -1; > > } > > > > -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { > > +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { > > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > _(" attribute 'display' is only supported" > > " with model='vfio-pci'")); > > @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const > > virDomainHostdevSubsysMediatedDev *mdevsrc, > > return -1; > > } > > > > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { > > +if (dev->display == VIR_TRISTATE_SWITCH_ON) { > > if (def->ngraphics == 0) { > > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > _("graphics device is needed for attribute > > value " > > @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const > > virDomainHostdevSubsysMediatedDev *mdevsrc, > > } > > > > > > +static int > > +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, > > + const virDomainDef *def, > > + virQEMUCapsPtr qemuCaps) > > +{ > > + > > +switch ((virMediatedDeviceModelType) mdevsrc->model) { > > +case VIR_MDEV_MODEL_TYPE_VFIO_PCI: > > +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); > > +case VIR_MDEV_MODEL_TYPE_VFIO_AP: > > +break; > > +case VIR_MDEV_MODEL_TYPE_VFIO_CCW: > > +break; > > +case VIR_MDEV_MODEL_TYPE_LAST: > > +virReportEnumRangeError(virMediatedDeviceModelType, > > +mdevsrc->model); > > +return -1; > > +} > > default case is missing? Otherwise looking good. Yeah, it could only happen due to memory corruption, but you're right we should stay both consistent and safe, consider it added. Thanks, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On Mon, Nov 12, 2018 at 01:14 PM +0100, Erik Skultety wrote: > Since we'll need to validate other models apart from VFIO PCI too, > having a helper for each model should keep the code base cleaner. > > Signed-off-by: Erik Skultety > --- > src/qemu/qemu_domain.c | 35 +-- > 1 file changed, 29 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index e25afdad6b..17d207513d 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const > virDomainNetDef *net) > > > static int > -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, > - const virDomainDef *def, > - virQEMUCapsPtr qemuCaps) > +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev > *dev, > + const virDomainDef *def, > + virQEMUCapsPtr qemuCaps) > { > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) > +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) > return 0; > > if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { > @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const > virDomainHostdevSubsysMediatedDev *mdevsrc, > return -1; > } > > -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { > +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _(" attribute 'display' is only supported" > " with model='vfio-pci'")); > @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const > virDomainHostdevSubsysMediatedDev *mdevsrc, > return -1; > } > > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { > +if (dev->display == VIR_TRISTATE_SWITCH_ON) { > if (def->ngraphics == 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _("graphics device is needed for attribute value " > @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const > virDomainHostdevSubsysMediatedDev *mdevsrc, > } > > > +static int > +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, > + const virDomainDef *def, > + virQEMUCapsPtr qemuCaps) > +{ > + > +switch ((virMediatedDeviceModelType) mdevsrc->model) { > +case VIR_MDEV_MODEL_TYPE_VFIO_PCI: > +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); > +case VIR_MDEV_MODEL_TYPE_VFIO_AP: > +break; > +case VIR_MDEV_MODEL_TYPE_VFIO_CCW: > +break; > +case VIR_MDEV_MODEL_TYPE_LAST: > +virReportEnumRangeError(virMediatedDeviceModelType, > +mdevsrc->model); > +return -1; > +} default case is missing? Otherwise looking good. > + > +return 0; > +} > + > + > static int > qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, > const virDomainDef *def, > -- > 2.19.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner. Signed-off-by: Erik Skultety --- src/qemu/qemu_domain.c | 35 +-- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _(" attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { +if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, } +static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + +switch ((virMediatedDeviceModelType) mdevsrc->model) { +case VIR_MDEV_MODEL_TYPE_VFIO_PCI: +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); +case VIR_MDEV_MODEL_TYPE_VFIO_AP: +break; +case VIR_MDEV_MODEL_TYPE_VFIO_CCW: +break; +case VIR_MDEV_MODEL_TYPE_LAST: +virReportEnumRangeError(virMediatedDeviceModelType, +mdevsrc->model); +return -1; +} + +return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def, -- 2.19.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list