From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH 1/6] libxl: libxl_device_disk_local_attach
 return a new libxl_device_disk
Date: Tue, 27 Mar 2012 17:21:38 +0100
Message-ID: <20337.59666.858188.849026@mariner.uk.xensource.com>
References: <alpine.DEB.2.00.1203271453390.15151@kaball-desktop>
	<1332856772-30292-1-git-send-email-stefano.stabellini@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1332856772-30292-1-git-send-email-stefano.stabellini@eu.citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Ian Campbell <Ian.Campbell@citrix.com>
List-Id: xen-devel@lists.xenproject.org

Stefano Stabellini writes ("[PATCH 1/6] libxl: libxl_device_disk_local_attach return a new libxl_device_disk"):
> -char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
> +char * libxl_device_disk_local_attach(libxl_ctx *ctx, const libxl_device_disk *disk,
> +        libxl_device_disk **new_disk)

Long line.

Shouldn't we make this a libxl-internal function ?  If we do that we
can allocate the new libxl_device_disk from the gc.

> +    if (tmpdisk->pdev_path != NULL)
> +        tmpdisk->pdev_path = strdup(tmpdisk->pdev_path);
> +    if (tmpdisk->script != NULL)
> +        tmpdisk->script = strdup(tmpdisk->script);
> +    tmpdisk->vdev = NULL;

Perhaps you should put my "malloc always fails" patch on the bottom of
your series ?  That means you can write
   tmpdisk->pdev_path = libxl__strdup(0, tmpdisk->pdev_path);
which will crash more sanely if malloc fails.

> -    switch (disk->backend) {
> +    switch (tmpdisk->backend) {

If you renamed the formal parameter rather than introducing a new
variable this would all be a lot easier to read (both the patch, and
the resulting code).

>   out:
> -    if (dev != NULL)
> +    if (dev != NULL) {
>          ret = strdup(dev);
> +        tmpdisk->vdev = strdup(tmpdisk->vdev);
> +    }
>      GC_FREE;
>      return ret;

This leaks tmpdisk if the function fails.  Or, alternatively, you
expect the caller to always free *new_disk even if _attach fails,
which is very counterintuitive.

This would be solved if you made this an internal function and used
the gc.

Ian.