| Message ID | 1450441425-10755-2-git-send-email-george.dunlap@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
George Dunlap writes ("[PATCH v4 1/5] libxl: Remove pointless hypercall from libxl_set_memory_target"):
> There's no obvious reason for the call to xc_domain_getinfolist -- all
> it seems to be doing is checking that the domain exists; but if it
> doesn't exist, it will have already failed by this point.
>
> NB that this will change the return value for libxl_set_memory_target:
> now it will return 0 on success, rather than returning 1 (which was
> the previous behavior). This is more in line with expected behavior,
> and also allows the caller to distingiush between success and other
> failure modes (some of which also return 1).
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Although I don't see the other return paths returning 1 which you
mention.
Ian.
On 04/01/16 17:28, Ian Jackson wrote: > George Dunlap writes ("[PATCH v4 1/5] libxl: Remove pointless hypercall from libxl_set_memory_target"): >> There's no obvious reason for the call to xc_domain_getinfolist -- all >> it seems to be doing is checking that the domain exists; but if it >> doesn't exist, it will have already failed by this point. >> >> NB that this will change the return value for libxl_set_memory_target: >> now it will return 0 on success, rather than returning 1 (which was >> the previous behavior). This is more in line with expected behavior, >> and also allows the caller to distingiush between success and other >> failure modes (some of which also return 1). > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > Although I don't see the other return paths returning 1 which you > mention. rc is initialized to 1, and there are a number of exit paths (I count 7) which jump to out without setting it; including setting the target above static_max. BTW I'm about to re-send the Ack'ed patches in this series (rebased to staging). -George
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 9207621..a36101d 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -4859,11 +4859,6 @@ retry_transaction: libxl__xs_printf(gc, t, GCSPRINTF("%s/memory/target", dompath), "%"PRIu32, new_target_memkb); - rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info); - if (rc != 1 || info.domain != domid) { - abort_transaction = 1; - goto out; - } libxl_dominfo_init(&ptr); xcinfo2xlinfo(ctx, &info, &ptr);
There's no obvious reason for the call to xc_domain_getinfolist -- all it seems to be doing is checking that the domain exists; but if it doesn't exist, it will have already failed by this point. NB that this will change the return value for libxl_set_memory_target: now it will return 0 on success, rather than returning 1 (which was the previous behavior). This is more in line with expected behavior, and also allows the caller to distingiush between success and other failure modes (some of which also return 1). Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- v4: - Split this change into a separate patch CC: Ian Campbell <ian.campbell@citrix.com> CC: Ian Jackson <ian.jackson@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- tools/libxl/libxl.c | 5 ----- 1 file changed, 5 deletions(-)