Re: [PATCH for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function

[Daniel P . Berrangé]

On Mon, Jul 20, 2020 at 01:01:32PM +0200, Thomas Huth wrote:
> We are going to support non-PCI devices soon. For this we need to split
> the generic GuestDiskAddress and GuestDiskAddressList memory allocation
> and chaining into a separate function first.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qga/commands-posix.c | 65 ++++++++++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1a62a3a70d..cddbaf5c69 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -861,28 +861,30 @@ static int build_hosts(char const *syspath, char const 
> *host, bool ata,
>      return i;
>  }
>  
> -/* Store disk device info specified by @sysfs into @fs */
> -static void build_guest_fsinfo_for_real_device(char const *syspath,
> -                                               GuestFilesystemInfo *fs,
> -                                               Error **errp)
> +/*
> + * Store disk device info for devices on the PCI bus.
> + * Returns true if information has been stored, or false for failure.
> + */
> +static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
> +                                           GuestDiskAddress *disk,
> +                                           GuestPCIAddress *pciaddr,
> +                                           Error **errp)
>  {
>      unsigned int pci[4], host, hosts[8], tgt[3];
>      int i, nhosts = 0, pcilen;
> -    GuestDiskAddress *disk;
> -    GuestPCIAddress *pciaddr;
> -    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
> +    bool ret = false;
>  
>      p = strstr(syspath, "/devices/pci");
>      if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
>                       pci, pci + 1, pci + 2, pci + 3, &pcilen) < 4) {
>          g_debug("only pci device is supported: sysfs path '%s'", syspath);
> -        return;
> +        return false;
>      }
>  
>      p += 12 + pcilen;
> @@ -903,7 +905,7 @@ static void build_guest_fsinfo_for_real_device(char const 
> *syspath,
>          }
>  
>          g_debug("unsupported driver or sysfs path '%s'", syspath);
> -        return;
> +        return false;
>      }
>  
>      p = strstr(syspath, "/target");
> @@ -929,18 +931,11 @@ static void build_guest_fsinfo_for_real_device(char 
> const *syspath,
>          }
>      }
>  
> -    pciaddr = g_malloc0(sizeof(*pciaddr));
>      pciaddr->domain = pci[0];
>      pciaddr->bus = pci[1];
>      pciaddr->slot = pci[2];
>      pciaddr->function = pci[3];
>  
> -    disk = g_malloc0(sizeof(*disk));
> -    disk->pci_controller = pciaddr;
> -
> -    list = g_malloc0(sizeof(*list));
> -    list->value = disk;
> -
>  #ifdef CONFIG_LIBUDEV
>      udev = udev_new();
>      udevice = udev_device_new_from_syspath(udev, syspath);
> @@ -1018,21 +1013,43 @@ static void build_guest_fsinfo_for_real_device(char 
> const *syspath,
>          goto cleanup;
>      }
>  
> -    list->next = fs->disk;
> -    fs->disk = list;
> -    goto out;
> +    ret = true;
>  
>  cleanup:
> -    if (list) {
> -        qapi_free_GuestDiskAddressList(list);
> -    }
> -out:
>      g_free(driver);
>  #ifdef CONFIG_LIBUDEV
>      udev_unref(udev);
>      udev_device_unref(udevice);
>  #endif
> -    return;
> +    return ret;
> +}
> +
> +/* Store disk device info specified by @sysfs into @fs */
> +static void build_guest_fsinfo_for_real_device(char const *syspath,
> +                                               GuestFilesystemInfo *fs,
> +                                               Error **errp)
> +{
> +    GuestDiskAddress *disk;
> +    GuestPCIAddress *pciaddr;
> +    GuestDiskAddressList *list = NULL;
> +    bool has_pci;
> +
> +    pciaddr = g_malloc(sizeof(*pciaddr));

g_new0 instead of g_malloc and thus kill the sizeof.

> +    memset(pciaddr, -1, sizeof(*pciaddr));  /* -1 means field is invalid */

Each field in GuestPCIAddress is an "int64_t", but memset works on bytes.

So you're not setting the fields to "-1" here, you're setting
each octet in the "int64_t" to -1.

> +
> +    disk = g_malloc0(sizeof(*disk));
> +    disk->pci_controller = pciaddr;
> +
> +    list = g_malloc0(sizeof(*list));
> +    list->value = disk;

g_new0 for these too.

(yes, I realize these were all pre-existing bugs)

> +
> +    has_pci = build_guest_fsinfo_for_pci_dev(syspath, disk, pciaddr, errp);
> +    if (has_pci) {
> +        list->next = fs->disk;
> +        fs->disk = list;
> +    } else {
> +        qapi_free_GuestDiskAddressList(list);
> +    }
>  }


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|