[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] 答复: Re: [PATCH v2] qemu_do main: Error out eariler when configuring IDE controller incor rectly




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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]