Re: [PATCH 1/2] qemu: Allow SMBIOS entries to be loaded and provided to the VM BIOS

[Anthony Liguori <aliguori@us.ibm.com>]

Alex Williamson wrote:
> Create a new -smbios options that takes binary SMBIOS entries
> to provide to the VM BIOS.  The binary can be easily generated
> using something like:
>
> dmidecode -t 1 -u | grep $'^\t\t[^"]' | xargs -n1 | \
> 	perl -lne 'printf "%c", hex($_)' > smbios_type_1.bin
>
> For some inventory tools, this makes the VM report the system
> information for the host.  One entry per binary file, multiple
> files can be chained together as:
>
>   -smbios file1,file2,...
>
> or specified independently:
>
>   -smbios file1 -smbios file2
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
>   

Hi Alex,

I know we have to support blobs because of OEM specific smbios entries, 
but there are a number of common ones that it would probably be good to 
specify in a less user-unfriendly way.  What do you think?

Anyway, comments below.

> diff --git a/hw/acpi.c b/hw/acpi.c
> index 52f50a0..0bd93bf 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -915,3 +915,69 @@ out:
>      }
>      return -1;
>  }
> +
> +char *smbios_entries;
> +size_t smbios_entries_len;
>   

I think an accessor would be better than making these variables global.

> +int smbios_entry_add(const char *t)
> +{
>   

acpi.c is hardware emulation, I'd rather see the command line parsing 
done somewhere else (like vl.c).

> +    struct stat s;
> +    char file[1024], *p, *f, *n;
> +    int fd, r;
> +    size_t len, off;
> +
> +    f = (char *)t;
> +    do {
> +        n = strchr(f, ',');
> +        if (n) {
> +            strncpy(file, f, (n - f));
> +            file[n - f] = '\0';
> +            f = n + 1;
> +        } else {
> +            strcpy(file, f);
> +            f += strlen(file);
> +        }
>   

I'm happy to just require multiple -smbios options.  I dislike 
overloading with ','s even though we do it a lot in QEMU.

> +        fd = open(file, O_RDONLY);
> +        if (fd < 0)
> +            return -1;
> +
> +        if (fstat(fd, &s) < 0) {
> +            close(fd);
> +            return -1;
> +        }
>   

May want to look at load_image/get_image_size.


-- 
Regards,

Anthony Liguori