[libvirt] [PATCH v2] qemu_domain: Error out eariler when configuring IDE controller incorrectly

[John Ferlan]

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

> 
> Signed-off-by: Lin Ma <lma at 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...

Your commit message only references moving, but you've added a 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,"

> +                                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"...

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

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

> +            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".

John

> +            goto cleanup;
>      }
>  
>      /* forbid capabilities mode hostdev in this kind of hypervisor */
>