From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach Date: Tue, 17 Apr 2012 18:58:22 +0100 Message-ID: <20365.44862.597858.180770@mariner.uk.xensource.com> References: <1334601190-14187-6-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: In-Reply-To: <1334601190-14187-6-git-send-email-stefano.stabellini@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: xen-devel@lists.xensource.com, Ian.Campbell@citrix.com List-Id: xen-devel@lists.xenproject.org Stefano Stabellini writes ("[Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach"): ... > + t = xs_transaction_start(ctx->xsh); This transaction should be in the local variables block for the whole function, and initialised to 0. > + /* use 0 as the domid of the toolstack domain for now */ > + tmpdisk->vdev = libxl__alloc_vdev(gc, 0, blkdev_start, t); Should this 0 be abstracted into a #define or a variable ? > + if (tmpdisk->vdev == NULL) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "libxl__alloc_vdev failed\n"); > + xs_transaction_end(ctx->xsh, t, 1); And then all these xs_transaction_end calls turn into one call in the exit path. > + dev = libxl__sprintf(gc, "/dev/%s", tmpdisk->vdev); Perhaps you'd like to use GCSPRINTF now that we have it. > LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n", > - disk->pdev_path); Perhaps you'd like to use LOG(DEBUG, ....") now that we have it ? > - dev = tmpdisk->pdev_path; > + switch (disk->backend) { > + case LIBXL_DISK_BACKEND_QDISK: > + if (disk->format != LIBXL_DISK_FORMAT_RAW) { This replicates the logic earlier, which decided whether to use a qdisk. I think it would be better to remember whether we did use a qdisk and detach it iff so. Ian.