From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH 1/6] libxl: libxl_device_disk_local_attach
 return a new libxl_device_disk
Date: Fri, 30 Mar 2012 11:43:14 +0100
Message-ID: <alpine.DEB.2.00.1203301142510.15151@kaball-desktop>
References: <alpine.DEB.2.00.1203271453390.15151@kaball-desktop>
	<1332856772-30292-1-git-send-email-stefano.stabellini@eu.citrix.com>
	<20337.59666.858188.849026@mariner.uk.xensource.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: <20337.59666.858188.849026@mariner.uk.xensource.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: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Tue, 27 Mar 2012, Ian Jackson wrote:
> 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.

Right, I'll do that.