Re: [PATCH v2 2/3] xl/libxl: Add ability to specify SMBIOS strings

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Sep 08, 2022 at 03:51:12PM -0400, Jason Andryuk wrote:
> hvm_xs_strings.h specifies xenstore entries which can be used to set or
> override smbios strings.  hvmloader has support for reading them, but
> xl/libxl support is not wired up.
> 
> Allow specifying the strings with the new xl.cfg option:
> smbios=["bios_vendor=Xen Project","system_version=1.0"]

I've tried something like:
    smbios=['oem=foo=bar']
And only "foo" is added to the smbios string. Shouldn't we allow users
to have '=' in their smbios strings?

> ---
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 2abaab439c..c3125ed310 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -771,6 +771,27 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
>              goto err;
>      }
>  
> +    for (int i = 0; i < info->u.hvm.num_smbios; i++) {
> +        char *p;
> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_BIOS_STRINGS"/%s", domid,
> +                   libxl_smbios_type_to_string(info->u.hvm.smbios[i].key));
> +
> +        /* Convert libxl_smbios_type string to xenstore path that hvmloader
> +           will use, as defined by HVM_XS_*. That is convert the '_' to '-'. 
> */

Could you add a '*' on the second line of that multi-line comment ?
    /* Multi-line
     * comments */

> +        p = strrchr(path, '/');
> +        for ( ; *p; p++) {
> +            if (*p == '_')
> +                *p = '-';
> +        }
> +
> +        LOGD(DEBUG, domid, "Writing %s = \"%s\"\n", path,

Could you remove the \n ? LOGD() macro already adds a new line.

> +             info->u.hvm.smbios[i].value);
> +        ret = libxl__xs_printf(gc, XBT_NULL, path, "%s",
> +                               info->u.hvm.smbios[i].value);
> +        if (ret)
> +            goto err;
> +    }
> +
>      /* Only one module can be passed. PVHv2 guests do not support this. */
>      if (dom->acpi_modules[0].guest_addr_out && 
>          info->type == LIBXL_DOMAIN_TYPE_HVM) {

The rest of the patch looks fine, even if I can't say much about the
generated golang code.

Thanks,

-- 
Anthony PERARD