From mboxrd@z Thu Jan  1 00:00:00 1970
From: Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v9 05/17] libxl: refactor disk addition to
	take a helper
Date: Fri, 20 Jul 2012 11:38:27 +0100
Message-ID: <50093523.30809@citrix.com>
References: <1342172696-88347-1-git-send-email-roger.pau@citrix.com>
	<1342172696-88347-6-git-send-email-roger.pau@citrix.com>
	<20485.38656.204680.313872@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: <20485.38656.204680.313872@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.xen.org" <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v9 05/17] libxl: refactor disk addition to take a helper"):
>> Change libxl__device_disk_add to no longer take a xs transaction and
>> instead pass a helper for the local attach case that's used to get the
>> free vdev.
> ...> 
>> This function contains some non-functional changes due to an
>> indentation change.
> 
>> +/* Specific function called directly only by local disk attach,
>> + * all other users should instead use the regular
>> + * libxl__device_disk_add wrapper
>> + */
>> +static int device_disk_add(libxl__gc *gc, uint32_t domid,
>> +                           libxl_device_disk *disk,
>> +                           char *(*fn)(libxl__gc *, const char *,
>> +                                              xs_transaction_t),
>> +                           const char *blkdev_start)
> 
> (I'm going to be quoting a diff -b.)
> 
> A better comment would be one which described what `fn' does.  Ie,
> rather than saying `this is internal to local attach', describe its
> semantics.
> 
> Maybe also `fn' could have a better name.  `get_vdev' ?
> 
> And the context pointer should be a void*, not a const char*.
> `void *get_vdev_user' or something.
> 
> ...
>> +        if (fn) {
>> +            assert(blkdev_start);
>> +            disk->vdev = fn(gc, blkdev_start, t);
>> +            if (disk->vdev == NULL) {
>> +                LOG(ERROR, "libxl__alloc_vdev failed");
> 
> Surely this logging is (a) not necessarily true since the caller's fn
> may have nothing to do with libxl__alloc_vdev (b) should be done by fn
> anyway, since fn probably knows what is going on.

I've fixed all the above, thanks for the review.