On 11/19/2017 08:58 AM, Lin Ma wrote:
>
>
>>>> John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
