<HTML><HEAD>
<META content="text/html; charset=utf-8" http-equiv=Content-Type>
<META name=GENERATOR content="MSHTML 8.00.7601.19038"></HEAD>
<BODY style="MARGIN: 4px 4px 1px; FONT: 10pt 微软雅黑">
<DIV><BR><BR>>>> John Ferlan <jferlan@redhat.com> 2017/11/10 星期五 上午 7:01 >>><BR>><BR>><BR>>On 10/23/2017 08:53 AM, Lin Ma wrote:<BR>>> Move error handling of IDE controller from qemuBuildControllerDevStr to<BR>>> qemuDomainDeviceDefValidate for reminding users eariler.<BR>><BR>>earlier<BR>ok, will fix it.<BR></DIV>
<DIV> </DIV>
<DIV>>> <BR>>> Signed-off-by: Lin Ma <lma@suse.com><BR>>> ---<BR>>> src/qemu/qemu_command.c | 17 -----------------<BR>>> src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++<BR>>> 2 files changed, 26 insertions(+), 17 deletions(-)<BR>>> <BR>><BR>>You would need to separate the *move* and *new* changes into separate<BR>>patches - makes it easier to review...<BR>The "move" only includes dropping code from src/qemu/qemu_command.c,</DIV>
<DIV>Can I understand in this way?</DIV>
<DIV><BR>>Your commit message only references moving, but you've added a q35<BR>>specific check.<BR>Patch v3 perhaps won't include q35 specific check.</DIV>
<DIV> </DIV>
<DIV><BR>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c<BR>>> index b1cfafa79..463952d9b 100644<BR>>> --- a/src/qemu/qemu_command.c<BR>>> +++ b/src/qemu/qemu_command.c<BR>>> @@ -3106,23 +3106,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,<BR>>> }<BR>>> break;<BR>>> <BR>>> - case VIR_DOMAIN_CONTROLLER_TYPE_IDE:<BR>>> - /* Since we currently only support the integrated IDE<BR>>> - * controller on various boards, if we ever get to here, it's<BR>>> - * because some other machinetype had an IDE controller<BR>>> - * specified, or one with a single IDE contraller had multiple<BR>>> - * ide controllers specified.<BR>>> - */<BR>>> - if (qemuDomainHasBuiltinIDE(domainDef))<BR>>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",<BR>>> - _("Only a single IDE controller is supported "<BR>>> - "for this machine type"));<BR>>> - else<BR>>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",<BR>>> - _("IDE controllers are unsupported for "<BR>>> - "this QEMU binary or machine type"));<BR>>> - goto error;<BR>>> -<BR>>> default:<BR>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,<BR>>> _("Unsupported controller type: %s"),<BR>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c<BR>>> index ece8ee7dd..d0be2afaf 100644<BR>>> --- a/src/qemu/qemu_domain.c<BR>>> +++ b/src/qemu/qemu_domain.c<BR>>> @@ -3539,6 +3539,29 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev,<BR>>> }<BR>>> <BR>>> <BR>>> +static int<BR>>> +qemuDomainControllerDefValidate(const virDomainControllerDefPtr controller,<BR>><BR>>make syntax-check would tell you:<BR>><BR>>forbid_const_pointer_typedef<BR>>src/qemu/qemu_domain.c:3582:qemuDomainControllerDefValidate(const<BR>>virDomainControllerDefPtr controller,<BR>>maint.mk: "const fooPtr var" does not declare what you meant<BR>><BR>>So this should be<BR>><BR>>"const virDomainControllerDef *controller,"<BR>ok, will fix it.</DIV>
<DIV> </DIV>
<DIV><BR>>> + const virDomainDef *def)<BR>>> +{<BR>>> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {<BR>>> + if (qemuDomainHasBuiltinIDE(def) && controller->idx != 0) {<BR>><BR>>OK this follows the checks from qemuBuildControllerDevCommandLine ...<BR>>while not necessarily straight from qemuBuildControllerDevStr, but it's<BR>>OK... Personally, I'd copy the comment as well, being sure to fix the<BR>>misspelled "contraller"...<BR>ok, will copy the comment and fix the typo.</DIV>
<DIV> </DIV>
<DIV><BR>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",<BR>>> + _("Only a single IDE controller is supported "<BR>>> + "for this machine type"));<BR>>> + return -1;<BR>>> + }<BR>><BR>>However, the else condition from qemuBuildControllerDevStr isn't fully<BR>>handled AFAICT...<BR>><BR>>The way qemuBuildControllerDevStr currently works for IDE is it won't be<BR>>called when:<BR>><BR>> /* first IDE controller is implicit on various machines */<BR>> if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&<BR>> cont->idx == 0 && !(def))<BR>> continue;<BR>><BR>>However, it would be called if "cont->idx == 0 &&<BR>>!qemuDomainHasBuiltinIDE" or when cont->idx > 0 regardless of BuiltinIDE.<BR>><BR>>Since this DefValidate routine will be called for each cont->idx, that<BR>>would say to me the former "else" turns into something like:<BR>><BR>> if (!qemuDomainHasBuiltinIDE(def)) {<BR>> _("IDE controllers are unsupported for "<BR>> "this QEMU binary or machine type"));<BR>> }<BR>><BR>>> + if (qemuDomainIsQ35(def)) {<BR>><BR>>This would seem to be a subset of the former else with a specific<BR>>machine specified.<BR>><BR>>So, the question I have is why is this being singled out? Does the<BR>>current code erroneously allow a q35 guest to have an IDE added to it on<BR>>the command line?<BR>I used to think that libvirt maybe will support adding IDE controllers in the future</DIV>
<DIV>for certain non-builtinIDE machine types, So only q35 is singled out.</DIV>
<DIV>It seems that my thought doesn't make sense.</DIV>
<DIV> </DIV>
<DIV><BR>>Maybe perhaps your patch could end up printing the machine type in the<BR>>"else" error message, e.g.<BR>><BR>> _("IDE controllers are unsupported for "<BR>> "this QEMU binary or machine type: %s"),<BR>> def->os.machine);<BR>If there already is 'IDE' section in guest xml, say:</DIV>
<DIV> <controller type='ide' index='0'><BR> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/><BR> </controller></DIV>
<DIV>Any valid changes about guest xml will trigger this "else" because the guest xml</DIV>
<DIV>matches "if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)".</DIV>
<DIV>Say removing a virtual disk or changing guest memory, they will trigger this "else"</DIV>
<DIV>to print this error message.</DIV>
<DIV>So, I'd like to change the code like these:</DIV>
<DIV>static int<BR>qemuDomainControllerDefValidate(const virDomainControllerDef *controller,<BR> const virDomainDef *def)<BR>{<BR> if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {<BR> /* Since we currently only support the integrated IDE<BR> * controller on various boards, if we ever get to here, it's<BR> * because some other machinetype had an IDE controller<BR> * specified, or one with a single IDE controller had multiple<BR> * ide controllers specified.<BR> */<BR> if (qemuDomainHasBuiltinIDE(def)) {<BR> if (controller->idx != 0) {<BR> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",<BR> _("Only a single IDE controller is supported "<BR> "for this machine type"));<BR> return -1;<BR> }<BR> }<BR> else {<BR> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,<BR> _("IDE controllers are unsupported for "<BR> "this machine type: %s"), def->os.machine);<BR> return -1;<BR> }<BR> }</DIV>
<DIV> </DIV>
<DIV> return 0;<BR>}</DIV>
<DIV>May I have your idea?</DIV>
<DIV> </DIV>
<DIV><BR>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",<BR>>> + _("IDE controllers are unsupported for q35 "<BR>>> + "machine type"));<BR>>> + return -1;<BR>>> + }<BR>>> + }<BR>>> +<BR>>> + return 0;<BR>>> +}<BR>>> +<BR>>> +<BR>>> static int<BR>>> qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,<BR>>> const virDomainDef *def,<BR>>> @@ -3650,6 +3673,9 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,<BR>>> } else if (dev->type == VIR_DOMAIN_DEVICE_WATCHDOG) {<BR>>> if (qemuDomainWatchdogDefValidate(dev->data.watchdog, def) < 0)<BR>>> goto cleanup;<BR>>> + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {<BR>>> + if (qemuDomainControllerDefValidate(dev->data.controller, def) < 0)<BR>><BR>>Since this is going through every controller anyway and theoretically<BR>>qemuBuildControllerDevStr would be called, it would seem reasonable to<BR>>perhaps move a number of the error checks into here and out of the<BR>>command line building... It's ambitious, but would seemingly be doable.<BR>><BR>>That would leave command line building only failing if some error was<BR>>done building the command line as opposed to also needing to check<BR>>whether whatever is about to be added is "valid".<BR>I'll try to figure out some error checks which are proper for moving into</DIV>
<DIV>here, But I'd like to do it in another patches, Is that ok?</DIV>
<DIV> </DIV>
<DIV>Thanks,</DIV>
<DIV>Lin</DIV></BODY></HTML>