Re: [PATCH v2 3/3] xl/libxl: Add OEM string support to smbios

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Sep 08, 2022 at 03:51:13PM -0400, Jason Andryuk wrote:
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index c3125ed310..0b01e09632 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -773,8 +774,18 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
>  
>      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));
> +        if (info->u.hvm.smbios[i].key == LIBXL_SMBIOS_TYPE_OEM) {
> +            if (num_oem > 99) {
> +                ret = ERROR_INVAL;

We probably want to log an error message here, to explain why libxl
returns an error.

> +                goto err;
> +            }
> +            path = GCSPRINTF("/local/domain/%d/"HVM_XS_OEM_STRINGS, domid,
> +                             num_oem);
> +            num_oem++;
> +        } else {
> +            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 '-'. 
> */
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 5413c36645..d0f8a14827 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1823,6 +1826,15 @@ void parse_config_data(const char *config_source,
>                  }
>                  free(option);
>  
> +                if (v == LIBXL_SMBIOS_TYPE_OEM) {
> +                    if (num_oem > 99) {
> +                        fprintf(stderr,
> +                                "xl: smbios limited to 99 oem strings\n");
> +                        exit(-ERROR_FAIL);

Could you change the exit value to be EXIT_FAILURE instead, like in the
other patch?
(ERROR_FAIL is meant to be an libxl return value and it somehow spread
to xl.)

Thanks,

-- 
Anthony PERARD