Re: [Qemu-devel] [PATCH v4 07/11] qga: report disk serial number

[Marc-André Lureau]

Hi

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <address@hidden> wrote:
>
> The feature is implemented for Windows and Linux. Reporting of serial
> number on Linux depends on libudev.
>
> Example from Linux:
>
>     {
>       "name": "dm-2",
>       "mountpoint": "/",
>       ...
>       "disk": [
>         {
>           "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
>           ...
>         }
>       ],
>     }
>
> Signed-off-by: Tomáš Golembiovský <address@hidden>
> ---
>  qga/Makefile.objs    |  1 +
>  qga/commands-posix.c | 27 +++++++++++++++++++++++++++
>  qga/commands-win32.c | 24 ++++++++++++++++++++++++
>  qga/qapi-schema.json |  4 +++-
>  4 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/qga/Makefile.objs b/qga/Makefile.objs
> index ed08c5917c..80e6bb3c2e 100644
> --- a/qga/Makefile.objs
> +++ b/qga/Makefile.objs
> @@ -1,3 +1,4 @@
> +commands-posix.o-libs := $(LIBUDEV_LIBS)
>  qga-obj-y = commands.o guest-agent-command-state.o main.o
>  qga-obj-$(CONFIG_POSIX) += commands-posix.o channel-posix.o
>  qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o service-win32.o
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 37e8a2d791..4d324178f2 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -48,6 +48,10 @@ extern char **environ;
>  #include <net/if.h>
>  #include <sys/statvfs.h>
>
> +#ifdef CONFIG_LIBUDEV
> +#include <libudev.h>
> +#endif
> +
>  #ifdef FIFREEZE
>  #define CONFIG_FSFREEZE
>  #endif
> @@ -872,6 +876,10 @@ static void build_guest_fsinfo_for_real_device(char 
> const *syspath,
>      GuestDiskAddressList *list = NULL;
>      bool has_ata = false, has_host = false, has_tgt = false;
>      char *p, *q, *driver = NULL;
> +#ifdef CONFIG_LIBUDEV
> +    struct udev *udev = NULL;
> +    struct udev_device *udevice = NULL;
> +#endif
>
>      p = strstr(syspath, "/devices/pci");
>      if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
> @@ -936,6 +944,21 @@ static void build_guest_fsinfo_for_real_device(char 
> const *syspath,
>      list = g_malloc0(sizeof(*list));
>      list->value = disk;
>
> +#ifdef CONFIG_LIBUDEV
> +    udev = udev_new();
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> +    if (udev == NULL || udevice == NULL) {
> +        g_debug("failed to query udev");
> +    } else {
> +        const char *serial;
> +        serial = udev_device_get_property_value(udevice, "ID_SERIAL");
> +        if (serial != NULL && *serial != 0) {
> +            disk->serial = g_strdup(serial);
> +            disk->has_serial = true;
> +        }
> +    }
> +#endif
> +
>      if (strcmp(driver, "ata_piix") == 0) {
>          /* a host per ide bus, target*:0:<unit>:0 */
>          if (!has_host || !has_tgt) {
> @@ -1003,6 +1026,10 @@ cleanup:

You correcly free udev context on error, but on normal return, it's leaking

You may want to change the return/cleanup path to have a common path.

(btw the leak is spotted by test/test-qga if you build with --enable-sanitizers)

>          qapi_free_GuestDiskAddressList(list);
>      }
>      g_free(driver);
> +#ifdef CONFIG_LIBUDEV
> +    udev_unref(udev);
> +    udev_device_unref(udevice);
> +#endif
>  }
>
>  static void build_guest_fsinfo_for_device(char const *devpath,
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d7864fc65a..376ca1e288 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -603,6 +603,30 @@ static void get_disk_properties(HANDLE vol_h, 
> GuestDiskAddress *disk,
>      }
>      disk->bus_type = find_bus_type(dev_desc->BusType);
>      g_debug("bus type %d", disk->bus_type);
> +
> +    /* Query once more. Now with long enough buffer. */
> +    size = dev_desc->Size;
> +    dev_desc = g_malloc0(size);
> +    if (!DeviceIoControl(vol_h, IOCTL_STORAGE_QUERY_PROPERTY, &query,
> +                         sizeof(STORAGE_PROPERTY_QUERY), dev_desc,
> +                         size, &received, NULL)) {
> +        error_setg_win32(errp, GetLastError(), "failed to get serial 
> number");
> +        goto out_free;
> +    }
> +    if (dev_desc->SerialNumberOffset > 0) {
> +        if (dev_desc->SerialNumberOffset >= received) {
> +            error_setg(errp, "offset outside the buffer");
> +            goto out_free;
> +        }
> +        const char *serial = (char *)dev_desc + dev_desc->SerialNumberOffset;
> +        size_t len = received - dev_desc->SerialNumberOffset;
> +        if (*serial != 0) {
> +            disk->serial = g_strndup(serial, len);
> +            disk->has_serial = true;
> +            g_debug("serial number %s", disk->serial);
> +        }
> +    }
> +out_free:
>      g_free(dev_desc);
>
>      return;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index dfbc4a5e32..3bcda6257e 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -834,13 +834,15 @@
>  # @bus: bus id
>  # @target: target id
>  # @unit: unit id
> +# @serial: serial number (since: 3.1)
>  #
>  # Since: 2.2
>  ##
>  { 'struct': 'GuestDiskAddress',
>    'data': {'pci-controller': 'GuestPCIAddress',
>             'bus-type': 'GuestDiskBusType',
> -           'bus': 'int', 'target': 'int', 'unit': 'int'} }
> +           'bus': 'int', 'target': 'int', 'unit': 'int',
> +           '*serial': 'str'} }
>
>  ##
>  # @GuestFilesystemInfo:
> --
> 2.19.0

looks good otherwise