[libvirt] [PATCH v2] qemu_domain: Error out eariler when configuring IDE controller incorrectly
Move error handling of IDE controller from qemuBuildControllerDevStr to qemuDomainDeviceDefValidate for reminding users eariler. Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b1cfafa79..463952d9b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3106,23 +3106,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } break; - case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - /* Since we currently only support the integrated IDE - * controller on various boards, if we ever get to here, it's - * because some other machinetype had an IDE controller - * specified, or one with a single IDE contraller had multiple - * ide controllers specified. - */ - if (qemuDomainHasBuiltinIDE(domainDef)) - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only a single IDE controller is supported " - "for this machine type")); - else - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IDE controllers are unsupported for " - "this QEMU binary or machine type")); - goto error; - default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller type: %s"), diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ece8ee7dd..d0be2afaf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3539,6 +3539,29 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev, } +static int +qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller, + const virDomainDef *def) +{ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { + if (qemuDomainHasBuiltinIDE(def) && controller->idx != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only a single IDE controller is supported " + "for this machine type")); + return -1; + } + if (qemuDomainIsQ35(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IDE controllers are unsupported for q35 " + "machine type")); + return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -3650,6 +3673,9 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, } else if (dev->type == VIR_DOMAIN_DEVICE_WATCHDOG) { if (qemuDomainWatchdogDefValidate(dev->data.watchdog, def) < 0) goto cleanup; + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + if (qemuDomainControllerDefValidate(dev->data.controller, def) < 0) + goto cleanup; } /* forbid capabilities mode hostdev in this kind of hypervisor */ -- 2.14.0
On 10/23/2017 08:53 AM, Lin Ma wrote:
Move error handling of IDE controller from qemuBuildControllerDevStr to qemuDomainDeviceDefValidate for reminding users eariler.
earlier
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 17 deletions(-)
You would need to separate the *move* and *new* changes into separate patches - makes it easier to review... Your commit message only references moving, but you've added a q35 specific check.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b1cfafa79..463952d9b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3106,23 +3106,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } break;
- case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - /* Since we currently only support the integrated IDE - * controller on various boards, if we ever get to here, it's - * because some other machinetype had an IDE controller - * specified, or one with a single IDE contraller had multiple - * ide controllers specified. - */ - if (qemuDomainHasBuiltinIDE(domainDef)) - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only a single IDE controller is supported " - "for this machine type")); - else - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IDE controllers are unsupported for " - "this QEMU binary or machine type")); - goto error; - default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller type: %s"), diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ece8ee7dd..d0be2afaf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3539,6 +3539,29 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev, }
+static int +qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller,
make syntax-check would tell you: forbid_const_pointer_typedef src/qemu/qemu_domain.c:3582:qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller, maint.mk: "const fooPtr var" does not declare what you meant So this should be "const virDomainControllerDef *controller,"
+ const virDomainDef *def) +{ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { + if (qemuDomainHasBuiltinIDE(def) && controller->idx != 0) {
OK this follows the checks from qemuBuildControllerDevCommandLine ... while not necessarily straight from qemuBuildControllerDevStr, but it's OK... Personally, I'd copy the comment as well, being sure to fix the misspelled "contraller"...
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only a single IDE controller is supported " + "for this machine type")); + return -1; + }
However, the else condition from qemuBuildControllerDevStr isn't fully handled AFAICT... The way qemuBuildControllerDevStr currently works for IDE is it won't be called when: /* first IDE controller is implicit on various machines */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && !(def)) continue; However, it would be called if "cont->idx == 0 && !qemuDomainHasBuiltinIDE" or when cont->idx > 0 regardless of BuiltinIDE. Since this DefValidate routine will be called for each cont->idx, that would say to me the former "else" turns into something like: if (!qemuDomainHasBuiltinIDE(def)) { _("IDE controllers are unsupported for " "this QEMU binary or machine type")); }
+ if (qemuDomainIsQ35(def)) {
This would seem to be a subset of the former else with a specific machine specified. So, the question I have is why is this being singled out? Does the current code erroneously allow a q35 guest to have an IDE added to it on the command line? Maybe perhaps your patch could end up printing the machine type in the "else" error message, e.g. _("IDE controllers are unsupported for " "this QEMU binary or machine type: %s"), def->os.machine);
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IDE controllers are unsupported for q35 " + "machine type")); + return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -3650,6 +3673,9 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, } else if (dev->type == VIR_DOMAIN_DEVICE_WATCHDOG) { if (qemuDomainWatchdogDefValidate(dev->data.watchdog, def) < 0) goto cleanup; + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + if (qemuDomainControllerDefValidate(dev->data.controller, def) < 0)
Since this is going through every controller anyway and theoretically qemuBuildControllerDevStr would be called, it would seem reasonable to perhaps move a number of the error checks into here and out of the command line building... It's ambitious, but would seemingly be doable. That would leave command line building only failing if some error was done building the command line as opposed to also needing to check whether whatever is about to be added is "valid". John
+ goto cleanup; }
/* forbid capabilities mode hostdev in this kind of hypervisor */
John Ferlan <jferlan@redhat.com> 2017/11/10 星期五 上午 7:01 >>>
On 10/23/2017 08:53 AM, Lin Ma wrote:
Move error handling of IDE controller from qemuBuildControllerDevStr to qemuDomainDeviceDefValidate for reminding users eariler.
earlier ok, will fix it.
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 17 deletions(-)
You would need to separate the *move* and *new* changes into separate patches - makes it easier to review... The "move" only includes dropping code from src/qemu/qemu_command.c, Can I understand in this way?
Your commit message only references moving, but you've added a q35 specific check. Patch v3 perhaps won't include q35 specific check.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b1cfafa79..463952d9b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3106,23 +3106,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } break;
- case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - /* Since we currently only support the integrated IDE - * controller on various boards, if we ever get to here, it's - * because some other machinetype had an IDE controller - * specified, or one with a single IDE contraller had multiple - * ide controllers specified. - */ - if (qemuDomainHasBuiltinIDE(domainDef)) - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only a single IDE controller is supported " - "for this machine type")); - else - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IDE controllers are unsupported for " - "this QEMU binary or machine type")); - goto error; - default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller type: %s"), diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ece8ee7dd..d0be2afaf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3539,6 +3539,29 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev, }
+static int +qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller,
make syntax-check would tell you:
forbid_const_pointer_typedef src/qemu/qemu_domain.c:3582:qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller, maint.mk: "const fooPtr var" does not declare what you meant
So this should be
"const virDomainControllerDef *controller," ok, will fix it.
+ const virDomainDef *def) +{ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { + if (qemuDomainHasBuiltinIDE(def) && controller->idx != 0) {
OK this follows the checks from qemuBuildControllerDevCommandLine ... while not necessarily straight from qemuBuildControllerDevStr, but it's OK... Personally, I'd copy the comment as well, being sure to fix the misspelled "contraller"... ok, will copy the comment and fix the typo.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only a single IDE controller is supported " + "for this machine type")); + return -1; + }
However, the else condition from qemuBuildControllerDevStr isn't fully handled AFAICT...
The way qemuBuildControllerDevStr currently works for IDE is it won't be called when:
/* first IDE controller is implicit on various machines */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && !(def)) continue;
However, it would be called if "cont->idx == 0 && !qemuDomainHasBuiltinIDE" or when cont->idx > 0 regardless of BuiltinIDE.
Since this DefValidate routine will be called for each cont->idx, that would say to me the former "else" turns into something like:
if (!qemuDomainHasBuiltinIDE(def)) { _("IDE controllers are unsupported for " "this QEMU binary or machine type")); }
+ if (qemuDomainIsQ35(def)) {
This would seem to be a subset of the former else with a specific machine specified.
So, the question I have is why is this being singled out? Does the current code erroneously allow a q35 guest to have an IDE added to it on the command line? I used to think that libvirt maybe will support adding IDE controllers in the future for certain non-builtinIDE machine types, So only q35 is singled out. It seems that my thought doesn't make sense.
Maybe perhaps your patch could end up printing the machine type in the "else" error message, e.g.
_("IDE controllers are unsupported for " "this QEMU binary or machine type: %s"), def->os.machine); If there already is 'IDE' section in guest xml, say: <controller type='ide' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> Any valid changes about guest xml will trigger this "else" because the guest xml matches "if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)". Say removing a virtual disk or changing guest memory, they will trigger this "else" to print this error message. So, I'd like to change the code like these: static int qemuDomainControllerDefValidate(const virDomainControllerDef *controller, const virDomainDef *def) { if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { /* Since we currently only support the integrated IDE * controller on various boards, if we ever get to here, it's * because some other machinetype had an IDE controller * specified, or one with a single IDE controller had multiple * ide controllers specified. */ if (qemuDomainHasBuiltinIDE(def)) { if (controller->idx != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only a single IDE controller is supported " "for this machine type")); return -1; } } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("IDE controllers are unsupported for " "this machine type: %s"), def->os.machine); return -1; } }
return 0; } May I have your idea?
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IDE controllers are unsupported for q35 " + "machine type")); + return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -3650,6 +3673,9 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, } else if (dev->type == VIR_DOMAIN_DEVICE_WATCHDOG) { if (qemuDomainWatchdogDefValidate(dev->data.watchdog, def) < 0) goto cleanup; + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + if (qemuDomainControllerDefValidate(dev->data.controller, def) < 0)
Since this is going through every controller anyway and theoretically qemuBuildControllerDevStr would be called, it would seem reasonable to perhaps move a number of the error checks into here and out of the command line building... It's ambitious, but would seemingly be doable.
That would leave command line building only failing if some error was done building the command line as opposed to also needing to check whether whatever is about to be added is "valid". I'll try to figure out some error checks which are proper for moving into here, But I'd like to do it in another patches, Is that ok?
Thanks, Lin
On 11/19/2017 08:58 AM, Lin Ma wrote:
John Ferlan <jferlan@redhat.com> 2017/11/10 星期五 上午 7:01 >>>
On 10/23/2017 08:53 AM, Lin Ma wrote:
Move error handling of IDE controller from qemuBuildControllerDevStr to qemuDomainDeviceDefValidate for reminding users eariler.
earlier ok, will fix it.
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_command.c | 17 ----------------- src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 17 deletions(-)
You would need to separate the *move* and *new* changes into separate patches - makes it easier to review... The "move" only includes dropping code from src/qemu/qemu_command.c, Can I understand in this way?
[sorry for the delay in responding - didn't realize there were multiple questions and I've been side tracked in other areas] Anyway - checking in qemuDomain*Validate for things that would then previously cause the qemu_command build to fail is perfectly reasonable. Making the *Validate check is preferred over the *PostParse type check since the latter could make guests disappear. So a pure move of code is fine... although pulling just the IDE check and leaving all the others is perhaps "less optimal" in the grand scheme of things.
Your commit message only references moving, but you've added a q35 specific check. Patch v3 perhaps won't include q35 specific check.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b1cfafa79..463952d9b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3106,23 +3106,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, } break; - case VIR_DOMAIN_CONTROLLER_TYPE_IDE: - /* Since we currently only support the integrated IDE - * controller on various boards, if we ever get to here, it's - * because some other machinetype had an IDE controller - * specified, or one with a single IDE contraller had multiple - * ide controllers specified. - */ - if (qemuDomainHasBuiltinIDE(domainDef)) - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only a single IDE controller is supported " - "for this machine type")); - else - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IDE controllers are unsupported for " - "this QEMU binary or machine type")); - goto error; - default:
BTW: Without a *TYPE_IDE: case above, we'd fall into this case...
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller type: %s"), diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ece8ee7dd..d0be2afaf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3539,6 +3539,29 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev, } +static int +qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller,
make syntax-check would tell you:
forbid_const_pointer_typedef src/qemu/qemu_domain.c:3582:qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller, maint.mk: "const fooPtr var" does not declare what you meant
So this should be
"const virDomainControllerDef *controller," ok, will fix it.
+ const virDomainDef *def) +{ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { + if (qemuDomainHasBuiltinIDE(def) && controller->idx != 0) {
OK this follows the checks from qemuBuildControllerDevCommandLine ... while not necessarily straight from qemuBuildControllerDevStr, but it's OK... Personally, I'd copy the comment as well, being sure to fix the misspelled "contraller"... ok, will copy the comment and fix the typo.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only a single IDE controller is supported " + "for this machine type")); + return -1; + }
However, the else condition from qemuBuildControllerDevStr isn't fully handled AFAICT...
The way qemuBuildControllerDevStr currently works for IDE is it won't be called when:
/* first IDE controller is implicit on various machines */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && !(def)) continue;
However, it would be called if "cont->idx == 0 && !qemuDomainHasBuiltinIDE" or when cont->idx > 0 regardless of BuiltinIDE.
Since this DefValidate routine will be called for each cont->idx, that would say to me the former "else" turns into something like:
if (!qemuDomainHasBuiltinIDE(def)) { _("IDE controllers are unsupported for " "this QEMU binary or machine type")); }
+ if (qemuDomainIsQ35(def)) {
This would seem to be a subset of the former else with a specific machine specified.
So, the question I have is why is this being singled out? Does the current code erroneously allow a q35 guest to have an IDE added to it on the command line? I used to think that libvirt maybe will support adding IDE controllers in the future for certain non-builtinIDE machine types, So only q35 is singled out. It seems that my thought doesn't make sense. > Maybe perhaps your patch could end up printing the machine type in the "else" error message, e.g.
_("IDE controllers are unsupported for " "this QEMU binary or machine type: %s"), def->os.machine); If there already is 'IDE' section in guest xml, say: <controller type='ide' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> Any valid changes about guest xml will trigger this "else" because the guest xml matches "if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)". Say removing a virtual disk or changing guest memory, they will trigger this "else" to print this error message. So, I'd like to change the code like these: static int qemuDomainControllerDefValidate(const virDomainControllerDef *controller, const virDomainDef *def) {
I think if you add the check from qemuBuildControllerDevCommandLine, such as: /* No need to check implicit/builtin IDE controller */ if (controller->idx == 0 && qemuDomainHasBuiltinIDE(def)) return 0; It'll make the subsequent check just a cut-n-paste from qemuBuildControllerDevStr. However, I think you'll find pseries and ccw tests begin to fail...
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { /* Since we currently only support the integrated IDE * controller on various boards, if we ever get to here, it's * because some other machinetype had an IDE controller * specified, or one with a single IDE controller had multiple * ide controllers specified. */ if (qemuDomainHasBuiltinIDE(def)) { if (controller->idx != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only a single IDE controller is supported " "for this machine type")); return -1; } } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("IDE controllers are unsupported for " "this machine type: %s"), def->os.machine); return -1; } } return 0; } May I have your idea?
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IDE controllers are unsupported for q35 " + "machine type")); + return -1; + } + } + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -3650,6 +3673,9 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, } else if (dev->type == VIR_DOMAIN_DEVICE_WATCHDOG) { if (qemuDomainWatchdogDefValidate(dev->data.watchdog, def) < 0) goto cleanup; + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + if (qemuDomainControllerDefValidate(dev->data.controller, def) < 0)
Since this is going through every controller anyway and theoretically qemuBuildControllerDevStr would be called, it would seem reasonable to perhaps move a number of the error checks into here and out of the command line building... It's ambitious, but would seemingly be doable.
That would leave command line building only failing if some error was done building the command line as opposed to also needing to check whether whatever is about to be added is "valid". I'll try to figure out some error checks which are proper for moving into here, But I'd like to do it in another patches, Is that ok? Thanks, Lin
I've been working on a model to move more checks to Validate today - I'm trying to clean/polish it up and will post. I've been stuck down the rabbit hole of USB and SCSI models having special cases \-( - it hasn't been clean... John
participants (2)
-
John Ferlan -
Lin Ma