[libvirt] [PATCH 1/6] conf: make hostdev info a separate object

[Eric Blake]

On 02/20/2012 10:10 AM, Laine Stump wrote:
> In order to allow for a virDomainHostdevDef that uses the
> virDomainDeviceInfo of a "higher level" device (such as a
> virDomainNetDef), this patch changes the virDomainDeviceInfo in the
> HostdevDef into a virDomainDeviceInfoPtr. Rather than adding checks
> all over the code to check for a null info, we just guarantee that it
> is always valid. The new function virDomainHostdevDefAlloc() allocates
> a virDomainDeviceInfo and plugs it in, and virDomainHostdevDefFree()
> makes sure it is freed.
> 
> There were 4 places allocating virDomainHostdevDefs, all of them
> parsers of one sort or another, and those have all had their
> VIR_ALLOC(hostdev) changed to virDomainHostdevDefAlloc(). Other than
> that, and the new functions, all the rest of the changes are just
> mechanical removals of "&" or changing "." to "->".

>  
> +virDomainHostdevDefPtr virDomainHostdevDefAlloc(void)
> +{
> +    virDomainHostdevDefPtr def = NULL;
> +
> +    if (VIR_ALLOC(def) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +    if (VIR_ALLOC(def->info) < 0) {
> +        VIR_FREE(def);
> +        return NULL;

Missing virReportOOMError() on this path.

> @@ -6527,15 +6527,17 @@ cleanup:
>  static virDomainHostdevDefPtr
>  qemuParseCommandLinePCI(const char *val)
>  {
> -    virDomainHostdevDefPtr def = NULL;
>      int bus = 0, slot = 0, func = 0;
>      const char *start;
>      char *end;
> +    virDomainHostdevDefPtr def = virDomainHostdevDefAlloc();
> +
> +    if (!def)
> +       goto cleanup;
>  
>      if (!STRPREFIX(val, "host=")) {
>          qemuReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("unknown PCI device syntax '%s'"), val);
> -        VIR_FREE(def);
>          goto cleanup;
>      }

Incomplete conversion - you nuked this VIR_FREE(def), but not the others
on the remaining error paths, nor did you fix the cleanup label to call
the correct new function to free things now that HostdevDef contains an
embedded pointer.  [And I have no idea why the VIR_FREE(def) was there
in the first place, given that pre-patch, def was initialized as NULL,
making the VIR_FREE a no-op]


> @@ -6584,11 +6581,14 @@ cleanup:
>  static virDomainHostdevDefPtr
>  qemuParseCommandLineUSB(const char *val)
>  {
> -    virDomainHostdevDefPtr def = NULL;
> +    virDomainHostdevDefPtr def = virDomainHostdevDefAlloc();
>      int first = 0, second = 0;
>      const char *start;
>      char *end;
>  
> +    if (!def)
> +       goto cleanup;
> +
>      if (!STRPREFIX(val, "host:")) {
>          qemuReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("unknown USB device syntax '%s'"), val);

Same problem here - you've left a (now-unsafe) VIR_FREE(def) in all the
error paths, instead of making cleanup: call a common function that
properly frees both def and its contents.

Overall, the idea looks reasonable, but you'll need a v2 to fix the
memory issues in qemu_command.c.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120220/3f0bd9c1/attachment-0001.sig>