Re: [Xen-devel] [PATCH v4 02/15] libxl: sanitize error handling in libxl_get_max_{cpus, nodes}

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-02 at 19:21 +0100, Dario Faggioli wrote:
> On mer, 2013-11-27 at 13:45 +0000, Ian Campbell wrote:
> > On Fri, 2013-11-22 at 19:56 +0100, Dario Faggioli wrote:
> > >  
> > >      cpumap->map = xc_cpupool_freeinfo(ctx->xch);
> > > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > > index 9f5f589..1815422 100644
> > > --- a/tools/libxl/libxl_utils.c
> > > +++ b/tools/libxl/libxl_utils.c
> > > @@ -651,6 +651,56 @@ char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, 
> > > const libxl_bitmap *bitmap)
> > >      return q;
> > >  }
> > >  
> > > +int libxl_cpu_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *cpumap, int 
> > > max_cpus)
> > 
> > You seem to have combined code motion with actual changes here. Please
> > don't do that.
> > 
> As I explained in the commit message for this patch in v5, I actually am
> both moving and (slightly) changing the functions, and that's mainly
> because the changes I'm doing require the move (e.g., GC_INIT/GC_FREE
> not being available in the original place).
> 
> I feel like it's more natural to do like this, rather than having a
> pre-patch just moving the code for no apparent reason... Isn't it?

Certainly making changes which are necessarily required by the code
motion is fine to do in a single step, although even then if you can
arrange to make the changes either before or after the move it is
better.

But is that what is happening here?

-static inline int libxl_cpu_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *cpumap,
-                                         int max_cpus)
-{
-    if (max_cpus < 0)
-        return ERROR_INVAL;
-    if (max_cpus == 0)
-        max_cpus = libxl_get_max_cpus(ctx);
-    if (max_cpus == 0)
-        return ERROR_FAIL;
-
-    return libxl_bitmap_alloc(ctx, cpumap, max_cpus);
-}

is becoming:

+int libxl_cpu_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *cpumap, int max_cpus)
+{
+    GC_INIT(ctx);
+    int rc = 0;
+
+    if (max_cpus < 0) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    if (max_cpus == 0)
+        max_cpus = libxl_get_max_cpus(ctx);
+    if (max_cpus <= 0) {
+        LOG(ERROR, "failed to retrieve the maximum number of cpus");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    /* This can't fail: no need to check and log */
+    libxl_bitmap_alloc(ctx, cpumap, max_cpus);
+
+ out:
+    GC_FREE;
+    return rc;
+}

which involves a lot more reworking than simply adding the GC. In any
case I don't see why the original function couldn't be moved as is and
then have the GC-ification added in the new location, I don't think the
move requires the change to using the GC In any way.


> Or was it something else that you were pointing out?
> 
> (Anyway, feel free to look at v5 and reply there).

In general it would be better to reply to vN either up front or as you
do the rework, so we can resolve any misunderstanding rather than
waiting until after you've posted vN+1 and perhaps requiring a vN+2.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel