>>> 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