Move the checks that various attributes are not set on any controller
other than SCSI controller using virtio-scsi model into the common
controller validate checks.
Need to also add a qemuDomainResetSCSIControllerModel call in order
to ensure we get the "right" SCSI model if it's not set by default
since it wouldn't be set during post parse processing.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/qemu/qemu_command.c | 24 ------------------------
src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5c084ae8c..1a9249e1b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2721,30 +2721,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
return -1;
}
- if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
- model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
- if (def->queues) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'queues' is only supported by virtio-scsi controller"));
- return -1;
- }
- if (def->cmd_per_lun) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'cmd_per_lun' is only supported by virtio-scsi controller"));
- return -1;
- }
- if (def->max_sectors) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'max_sectors' is only supported by virtio-scsi controller"));
- return -1;
- }
- if (def->ioeventfd) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'ioeventfd' is only supported by virtio-scsi controller"));
- return -1;
- }
- }
-
switch ((virDomainControllerType) def->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
switch (model) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index de46ab996..1a890c01a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3941,6 +3941,38 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk)
static int
+qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller,
+ int model)
+{
+ if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
+ model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
+ if (controller->queues) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'queues' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ if (controller->cmd_per_lun) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'cmd_per_lun' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ if (controller->max_sectors) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'max_sectors' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ if (controller->ioeventfd) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'ioeventfd' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
+static int
qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controller,
const virDomainDef *def)
{
@@ -3972,11 +4004,20 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
virQEMUCapsPtr qemuCaps)
{
int ret = 0;
+ int model = controller->model;
if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps,
"controller"))
return -1;
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, &model)) < 0)
+ return -1;
+ }
+
+ if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0)
+ return -1;
+
switch ((virDomainControllerType) controller->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
ret = qemuDomainDeviceDefValidateControllerIDE(controller, def);
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/06/2018 12:47 AM, John Ferlan wrote: > Move the checks that various attributes are not set on any controller > other than SCSI controller using virtio-scsi model into the common > controller validate checks. > > Need to also add a qemuDomainResetSCSIControllerModel call in order > to ensure we get the "right" SCSI model if it's not set by default > since it wouldn't be set during post parse processing. > > Signed-off-by: John Ferlan <jferlan@redhat.com> > --- > src/qemu/qemu_command.c | 24 ------------------------ > src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 24 deletions(-) The only problem I have with this approach is that while previously we've checked for QEMU caps at domain start time, now we check for them at define time. So I guess in general it's not a safe thing to do. For instance, I'd be against moving all checks done at cmd line time to DefPostParse as they introduce TOCTOU problem. However, some checks (mostly semantic ones) can be done in post parse callbacks. For example, trying to plug a disk onto ISA bus will fail regardless of qemu caps. However, whether qemu supports VIRTIO_SCSI or not should not matter at define time as this might change after domain is defined. However, SCSI controllers have been around for quite some time, so unless somebody is upgrading from ancient qemu, we are safe. ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Jan 28, 2018 at 09:48:19AM +0100, Michal Privoznik wrote: >On 01/06/2018 12:47 AM, John Ferlan wrote: >> Move the checks that various attributes are not set on any controller >> other than SCSI controller using virtio-scsi model into the common >> controller validate checks. >> >> Need to also add a qemuDomainResetSCSIControllerModel call in order >> to ensure we get the "right" SCSI model if it's not set by default >> since it wouldn't be set during post parse processing. >> >> Signed-off-by: John Ferlan <jferlan@redhat.com> >> --- >> src/qemu/qemu_command.c | 24 ------------------------ >> src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 41 insertions(+), 24 deletions(-) > > >The only problem I have with this approach is that while previously >we've checked for QEMU caps at domain start time, now we check for them >at define time. So I guess in general it's not a safe thing to do. > >For instance, I'd be against moving all checks done at cmd line time to >DefPostParse as they introduce TOCTOU problem. This is not DefPostParse, this is DefValidate. PostParse is called on all XML parsing (and as of <7c5cf4983> allowed to fail and then re-run on domain startup). Validate is run when defining some new domains (the _VALIDATE flag has to be added to the APIs) and then unconditionally on domain startup. So the only problem here would be that we might not allow to define a domain until you install QEMU with the requested features. >However, some checks >(mostly semantic ones) can be done in post parse callbacks. For example, >trying to plug a disk onto ISA bus will fail regardless of qemu caps. >However, whether qemu supports VIRTIO_SCSI or not should not matter at >define time as this might change after domain is defined. > >However, SCSI controllers have been around for quite some time, so >unless somebody is upgrading from ancient qemu, we are safe. > Generally we try not to break parsing existing configs (even with unusable domains), which is why Validate functions are separate from PostParse. Jan >ACK > >Michal > >-- >libvir-list mailing list >libvir-list@redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/29/2018 04:28 AM, Ján Tomko wrote: > On Sun, Jan 28, 2018 at 09:48:19AM +0100, Michal Privoznik wrote: >> On 01/06/2018 12:47 AM, John Ferlan wrote: >>> Move the checks that various attributes are not set on any controller >>> other than SCSI controller using virtio-scsi model into the common >>> controller validate checks. >>> >>> Need to also add a qemuDomainResetSCSIControllerModel call in order >>> to ensure we get the "right" SCSI model if it's not set by default >>> since it wouldn't be set during post parse processing. >>> >>> Signed-off-by: John Ferlan <jferlan@redhat.com> >>> --- >>> src/qemu/qemu_command.c | 24 ------------------------ >>> src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 41 insertions(+), 24 deletions(-) >> >> >> The only problem I have with this approach is that while previously >> we've checked for QEMU caps at domain start time, now we check for them >> at define time. So I guess in general it's not a safe thing to do. >> >> For instance, I'd be against moving all checks done at cmd line time to >> DefPostParse as they introduce TOCTOU problem. > > This is not DefPostParse, this is DefValidate. > > PostParse is called on all XML parsing (and as of <7c5cf4983> allowed > to fail and then re-run on domain startup). > > Validate is run when defining some new domains (the _VALIDATE flag has > to be added to the APIs) and then unconditionally on domain startup. > While I didn't go chase the commit for validate to be enabled, this is essentially my understanding of validate too. Callbacks that while could be in post parse callbacks, we don't put there because we don't want running domains to disappear. The whole post parse and validate and flags can be really confusing.... > So the only problem here would be that we might not allow to define > a domain until you install QEMU with the requested features. > True, but I don't believe it wouldn't be a running domain that would disappear as opposed to if these were post parse callback checks. Conversely this may "help" in the detection of some invalid configuration on a future change and test at XML2XML processing time rather than XML2ARGV (and don't really run the domain) time. >> However, some checks >> (mostly semantic ones) can be done in post parse callbacks. For example, >> trying to plug a disk onto ISA bus will fail regardless of qemu caps. >> However, whether qemu supports VIRTIO_SCSI or not should not matter at >> define time as this might change after domain is defined. >> >> However, SCSI controllers have been around for quite some time, so >> unless somebody is upgrading from ancient qemu, we are safe. >> > > Generally we try not to break parsing existing configs (even with > unusable domains), which is why Validate functions are separate > from PostParse. > > Jan > >> ACK >> >> Michal Having the ACK and an open question/issue is making me "pause"... I'd like to resolve Jan's other point as well, but I'll respond to it separately. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote: >Move the checks that various attributes are not set on any controller >other than SCSI controller using virtio-scsi model into the common >controller validate checks. > >Need to also add a qemuDomainResetSCSIControllerModel call in order >to ensure we get the "right" SCSI model if it's not set by default >since it wouldn't be set during post parse processing. > So we should set it in post-parse processing once instead of adding this second call. Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/29/2018 04:18 AM, Ján Tomko wrote: > On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote: >> Move the checks that various attributes are not set on any controller >> other than SCSI controller using virtio-scsi model into the common >> controller validate checks. >> >> Need to also add a qemuDomainResetSCSIControllerModel call in order >> to ensure we get the "right" SCSI model if it's not set by default >> since it wouldn't be set during post parse processing. >> > > So we should set it in post-parse processing once instead of adding this > second call. > > Jan This is a point you raised in v3: https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html to which I responded: https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html and even carried into my v4: https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html So, now that we're here again - the question becomes, do we really want post parse processing to set a default model if it's not set? Should that be part of this series? Should it be a followup? Should be something done before this one and then rework this one? Are there drawbacks to doing so? One drawback that I kept coming back to is that LSI becomes the default qemu model instead of VIRTIO-SCSI (it's the order of setting *model in qemuDomainSetSCSIControllerModel). That drawback caused problems when setting the model if we create a new SCSI controller during something like hotplug because we had already filled the current controller or when adding one VIRTIO-SCSI controller but more than 7 disks (or hostdevs) that end up using that controller. Although I think that's at least partially resolved by other changes I've already made. Tks - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 29, 2018 at 12:43:14PM -0500, John Ferlan wrote: > > >On 01/29/2018 04:18 AM, Ján Tomko wrote: >> On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote: >>> Move the checks that various attributes are not set on any controller >>> other than SCSI controller using virtio-scsi model into the common >>> controller validate checks. >>> >>> Need to also add a qemuDomainResetSCSIControllerModel call in order >>> to ensure we get the "right" SCSI model if it's not set by default >>> since it wouldn't be set during post parse processing. >>> >> >> So we should set it in post-parse processing once instead of adding this >> second call. >> >> Jan > >This is a point you raised in v3: > >https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html > >to which I responded: > >https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html > >and even carried into my v4: > >https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html > >So, now that we're here again - the question becomes, do we really want >post parse processing to set a default model if it's not set? Yes, the used model should be recorded in the XML. >Should >that be part of this series? Should it be a followup? Should be >something done before this one and then rework this one? > Ideally a prerequisite. If that's too much yak shaving for you, at least add a comment to the validate function saying picking a default model really does not belong in a validation function. >Are there drawbacks to doing so? One drawback that I kept coming back >to is that LSI becomes the default qemu model instead of VIRTIO-SCSI >(it's the order of setting *model in qemuDomainSetSCSIControllerModel). When was virtio-scsi the default model? Now we'd just record the picked default in XML. Jan >That drawback caused problems when setting the model if we create a new >SCSI controller during something like hotplug because we had already >filled the current controller or when adding one VIRTIO-SCSI controller >but more than 7 disks (or hostdevs) that end up using that controller. >Although I think that's at least partially resolved by other changes >I've already made. > >Tks - > >John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/29/2018 12:53 PM, Ján Tomko wrote: > On Mon, Jan 29, 2018 at 12:43:14PM -0500, John Ferlan wrote: >> >> >> On 01/29/2018 04:18 AM, Ján Tomko wrote: >>> On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote: >>>> Move the checks that various attributes are not set on any controller >>>> other than SCSI controller using virtio-scsi model into the common >>>> controller validate checks. >>>> >>>> Need to also add a qemuDomainResetSCSIControllerModel call in order >>>> to ensure we get the "right" SCSI model if it's not set by default >>>> since it wouldn't be set during post parse processing. >>>> >>> >>> So we should set it in post-parse processing once instead of adding this >>> second call. >>> >>> Jan >> >> This is a point you raised in v3: >> >> https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html >> >> to which I responded: >> >> https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html >> >> and even carried into my v4: >> >> https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html >> >> So, now that we're here again - the question becomes, do we really want >> post parse processing to set a default model if it's not set? > > Yes, the used model should be recorded in the XML. > >> Should >> that be part of this series? Should it be a followup? Should be >> something done before this one and then rework this one? >> > > Ideally a prerequisite. If that's too much yak shaving for you, > at least add a comment to the validate function saying picking a default > model really does not belong in a validation function. > As usual a seemingly simple request sends me down the rabbit hole. Setting a default controller model in qemuDomainControllerDefPostParse would require passing @qemuCaps - simple enough until one checks out the caller qemuDomainDeviceDefPostParse which notes: " /* Note that qemuCaps may be NULL when this function is called. This * function shall not fail in that case. It will be re-run on VM startup * with the capabilities populated. */ " <sigh> So, would adding the following to patch 1 for the Reset function suffice? /** qemuDomainResetSCSIControllerModel * @def: Domain def * @qemuCaps: QEMU capabilities * @model: model to reset "default" to - either by @def or @qemuCaps * * Typically qemuDomainControllerDefPostParse would be used to set * a default controller model; however, qemuDomainDeviceDefPostParse * can be passed a NULL @qemuCaps, so setting the default model may * not be possible. Thus we're left to calling this Reset function * from numerous locations that need to get the default model for * the controller when one is not defined. * * Returns 0 on success, -1 on failure */ I can also add a note in qemuDomainDeviceDefValidateController that restates this for this patch, e.g.: if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { /* Resetting the default should have been handled during post parse * parse callback; however, at that time we could not guarantee that * qemuCaps was valid, so we're left doing this now */ if ((qemuDomainResetSCSIControllerModel(def, qemuCaps, &model)) < 0) return -1; } >> Are there drawbacks to doing so? One drawback that I kept coming back >> to is that LSI becomes the default qemu model instead of VIRTIO-SCSI >> (it's the order of setting *model in qemuDomainSetSCSIControllerModel). > > When was virtio-scsi the default model? Now we'd just record the > picked default in XML. virtio-scsi was never the default AFAIK... That wasn't the point/problem. The problem is described in the next paragraph. > > Jan > >> That drawback caused problems when setting the model if we create a new >> SCSI controller during something like hotplug because we had already >> filled the current controller or when adding one VIRTIO-SCSI controller >> but more than 7 disks (or hostdevs) that end up using that controller. >> Although I think that's at least partially resolved by other changes >> I've already made. I looked it up, this is handled by c52dbafe and 07beea6c John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 29, 2018 at 16:18:46 -0500, John Ferlan wrote: > > > On 01/29/2018 12:53 PM, Ján Tomko wrote: > > On Mon, Jan 29, 2018 at 12:43:14PM -0500, John Ferlan wrote: > >> > >> > >> On 01/29/2018 04:18 AM, Ján Tomko wrote: > >>> On Fri, Jan 05, 2018 at 06:47:25PM -0500, John Ferlan wrote: > >>>> Move the checks that various attributes are not set on any controller > >>>> other than SCSI controller using virtio-scsi model into the common > >>>> controller validate checks. > >>>> > >>>> Need to also add a qemuDomainResetSCSIControllerModel call in order > >>>> to ensure we get the "right" SCSI model if it's not set by default > >>>> since it wouldn't be set during post parse processing. > >>>> > >>> > >>> So we should set it in post-parse processing once instead of adding this > >>> second call. > >>> > >>> Jan > >> > >> This is a point you raised in v3: > >> > >> https://www.redhat.com/archives/libvir-list/2017-December/msg00293.html > >> > >> to which I responded: > >> > >> https://www.redhat.com/archives/libvir-list/2017-December/msg00304.html > >> > >> and even carried into my v4: > >> > >> https://www.redhat.com/archives/libvir-list/2018-January/msg00103.html > >> > >> So, now that we're here again - the question becomes, do we really want > >> post parse processing to set a default model if it's not set? > > > > Yes, the used model should be recorded in the XML. > > > >> Should > >> that be part of this series? Should it be a followup? Should be > >> something done before this one and then rework this one? > >> > > > > Ideally a prerequisite. If that's too much yak shaving for you, > > at least add a comment to the validate function saying picking a default > > model really does not belong in a validation function. Well it really should not be there. I've tried to make sure that at least the top level functions get a const domain definition, so that nobody even thinks about doing this. Obviously for members of the domain object this can't be enforced by the compiler. Only via code review. This is the prototype for the validator: static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque) Do NOT try to set stuff there. The functions are meant only to check config. > > > > As usual a seemingly simple request sends me down the rabbit hole. > Setting a default controller model in qemuDomainControllerDefPostParse > would require passing @qemuCaps - simple enough until one checks out the > caller qemuDomainDeviceDefPostParse which notes: > > " > /* Note that qemuCaps may be NULL when this function is called. This > * function shall not fail in that case. It will be re-run on VM startup > * with the capabilities populated. */ I think I've seen this explained recently somewhere but I can reiterate. The above statement applies to cases when the postparse callback is called on code paths which should not fail. This includes reloading configs from disks and a few other things. qemuCaps will be NULL if the emulator binary is not present. We can't do much in that case, but we certainly should not drop domains. Defining new domains still requires qemuCaps to be populated so that defaults can be loaded according to the qemu capabilities. In cases when qemuCaps are NULL most of the defaults should be present. In cases when we would do this during daemon upgrade, post parse will be re-run during VM startup when qemuCaps are required to be present anyways. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...] >>> Ideally a prerequisite. If that's too much yak shaving for you, >>> at least add a comment to the validate function saying picking a default >>> model really does not belong in a validation function. > > Well it really should not be there. I've tried to make sure that at > least the top level functions get a const domain definition, so that > nobody even thinks about doing this. > > Obviously for members of the domain object this can't be enforced by the > compiler. Only via code review. > > > This is the prototype for the validator: > > static int > qemuDomainDefValidate(const virDomainDef *def, > virCapsPtr caps ATTRIBUTE_UNUSED, > void *opaque) > > > Do NOT try to set stuff there. The functions are meant only to check > config. > Have no worries, the patches don't "set" the def->controller[n]->model value for SCSI controllers that don't have one set... If they did there would be a bunch of make check failures because a value would be set and thus alter output in some xml file the xml2xml testing. /me wondering if a make check rule should be made to ensure that const never changes... >>> >> >> As usual a seemingly simple request sends me down the rabbit hole. >> Setting a default controller model in qemuDomainControllerDefPostParse >> would require passing @qemuCaps - simple enough until one checks out the >> caller qemuDomainDeviceDefPostParse which notes: >> >> " >> /* Note that qemuCaps may be NULL when this function is called. This >> * function shall not fail in that case. It will be re-run on VM startup >> * with the capabilities populated. */ > > I think I've seen this explained recently somewhere but I can reiterate. > > The above statement applies to cases when the postparse callback is > called on code paths which should not fail. This includes reloading > configs from disks and a few other things. > > qemuCaps will be NULL if the emulator binary is not present. We can't do > much in that case, but we certainly should not drop domains. Right, but if we cannot be sure that post parse would set the default, then I would think paths that possibly rely on getting a caps based model value cannot assume that the model was set during post parse. IOW: They'd have to adjust too - as seen with the USB and Net model code paths. > > Defining new domains still requires qemuCaps to be populated so that > defaults can be loaded according to the qemu capabilities. In cases when > qemuCaps are NULL most of the defaults should be present. In cases when > we would do this during daemon upgrade, post parse will be re-run during > VM startup when qemuCaps are required to be present anyways. > Does today's truth hold true to some future? The API mentions that qemuCaps "may" be NULL, but provides no real guidance over what assumptions can be made by not setting something based on not having the capabilities existing. In any case, I'm working through some patches - let's see which rabbit hole I get stuck in. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.