Re: [PATCH v2 2/6] bhyve: add bhyveDomainDefNeedsISAController helper

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Sun, Feb 17, 2019 at 05:04:01PM +0400, Roman Bogorodskiy wrote:
> Add a bhyveDomainDefNeedsISAController() helper function
> which by domain configuration determines whether LPC controller is
> required or not.
>
> Signed-off-by: Roman Bogorodskiy <bogorodskiy@xxxxxxxxx>
> ---
> src/bhyve/bhyve_command.c |  5 +----
> src/bhyve/bhyve_domain.c  | 10 ++++++++++
> src/bhyve/bhyve_domain.h  |  2 ++
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index 1f215dac08..f49dc77118 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -460,7 +460,6 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
>      *            vm0
>      */
>     size_t i;
> -    bool add_lpc = false;
>     int nusbcontrollers = 0;
>     unsigned int nvcpus = virDomainDefGetVcpus(def);
>
> @@ -549,7 +548,6 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
>         if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM)) {
>             virCommandAddArg(cmd, "-l");
>             virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->path);
> -            add_lpc = true;
>         } else {
>             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                            _("Installed bhyve binary does not support "
> @@ -613,7 +611,6 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
>             if (bhyveBuildGraphicsArgStr(def, def->graphics[0], def->videos[0],
>                                          conn, cmd, dryRun) < 0)
>                 goto error;
> -            add_lpc = true;
>         } else {
>             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                            _("Multiple graphics devices are not supported"));
> @@ -621,7 +618,7 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
>         }
>     }
>
> -    if (add_lpc || def->nserials)
> +    if (bhyveDomainDefNeedsISAController(def))
>         bhyveBuildLPCArgStr(def, cmd);
>
>     if (bhyveBuildConsoleArgStr(def, cmd) < 0)
> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
> index 79cf103d28..e476ef7e7d 100644
> --- a/src/bhyve/bhyve_domain.c
> +++ b/src/bhyve/bhyve_domain.c
> @@ -61,6 +61,16 @@ virDomainXMLPrivateDataCallbacks virBhyveDriverPrivateDataCallbacks = {
>     .free = bhyveDomainObjPrivateFree,
> };
>
> +bool
> +bhyveDomainDefNeedsISAController(virDomainDefPtr def)
> +{
> +    if ((def->os.bootloader == NULL && def->os.loader) ||
> +        (def->nconsoles || def->nserials) || (def->ngraphics && def->nvideos))
> +        return true;

Where did nconsoles come from?

Also, since it's a separate function, you can use return more:
   if (def->os.bootloader == NULL && def->os.loader)
       return true;
   if (def->nserials)
       return true;

> +    else
> +        return false;
> +}
> +
> static int
> bhyveDomainDefPostParse(virDomainDefPtr def,
>                         virCapsPtr caps ATTRIBUTE_UNUSED,

With nconsoles removed:
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature