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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

`On mar, 2013-12-03 at 09:41 +0000, Ian Campbell wrote:
> On Mon, 2013-12-02 at 19:21 +0100, Dario Faggioli wrote:
> > 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. 
>
Well, is it? All I'm doing is adding the GC and one LOG(). In v5 it's 2
LOG()s. The rest of the rework is indeed related to the GC-ification,
since I can't leave without calling GC_FREE anymore...

> 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.
> 
Mmm... I'm not sure I'm fully understanding what you're trying to say.
That function is in libxl_utils.h right now (IIRC, I put it there
myself :-)) because it's a trivial wrapper of libxl_bitmap_alloc().

What is happening is that, as a result of changing the error handling in
libxl_get_max_cpus(), and deciding to move logging for when it fails
closer to it --which is what happens in this very patch-- I just don't
think this is a trivial enough wrapper any longer.

So, my view here is: if I modify the function, adding GC and the LOG()s,
then it should also be moved, if not, it can stay where it is. What you
seem to be suggesting is that I (either in this or a previous patch)
move the function as is, for no apparent reason, and then (either in a
following or this patch), introduce the GC and the LOG()s... Is that the
case, or am I missing something? I see the point of not combining
functional and not-functional changes, but that really looks odd to me,
in this particular case.

Anyway, if that's what you want, I certainly can do that... I'm not
being graded against a maximum number of patches in a series, am I? ;-P

> > (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.
> 
Definitely, and I always do that. This round suffered from a combination
of weird circumstances on my side. Sorry for that.

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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