6:21 p.m.
On 11/19/2017 08:58 AM, Lin Ma wrote:
>>> John Ferlan <jferlan(a)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(a)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