| Message ID | 20191224151932.6304-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support CPUID/MSR data in migration streams | expand |
Andrew Cooper writes ("[PATCH 01/12] libxc/save: Shrink code volume where possible"):
> A property of how the error handling (0 on success, nonzero otherwise)
> allows these calls to be chained together with the ternary operatior.
I'm quite surprised to find a suggestion like this coming from you in
particular. I think if we are going to adopt this thing in general,
it ought to be in a CODING_STYLE somewhere.
I'm distinctly unsure about the merits of the pattern. It does make
the code much shorter and less repetitive. OTOH ?: is a
not-very-frequently used GNU extension and my representative sample of
programmers had to think about what this idiom meant and it wasn't
universally liked. On the third hand, if this idiom becomes dominant
you only have to think about it once.
Maybe it would be better to have
#define MUST(call) ({ rc = (call); if (rc) goto error; })
and write
MUST( write_one_vcpu_basic(ctx, i) );
Or just to permit
rc = write_one_vcpu_basic(ctx, i); if (rc) goto error;
(ie on a single line).
Ian.
Ian Jackson writes ("Re: [PATCH 01/12] libxc/save: Shrink code volume where possible"):
> Maybe it would be better to have
> #define MUST(call) ({ rc = (call); if (rc) goto error; })
> and write
> MUST( write_one_vcpu_basic(ctx, i) );
This is not uncommon. BIND9 does something like it:
https://git.uis.cam.ac.uk/x/uis/ipreg/bind9.git/blob/HEAD:/lib/dns/zone.c#l515
A friend points out that
#define MUST(x) ({ int rc_ = (x); if (rc_) { rc = rc_; goto error; } })
is better because it keeps rc uninitialised until the last moment.
That means the compiler can spot exit paths where you fail to set rc.
Ian.
On 14/01/2020 16:48, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH 01/12] libxc/save: Shrink code volume where possible"): >> A property of how the error handling (0 on success, nonzero otherwise) >> allows these calls to be chained together with the ternary operatior. > I'm quite surprised to find a suggestion like this coming from you in > particular. What probably is relevant is that ?: is a common construct in the hypervisor, which I suppose does colour my expectation of people knowing exactly what it means and how it behaves. > Maybe it would be better to have > #define MUST(call) ({ rc = (call); if (rc) goto error; }) > and write > MUST( write_one_vcpu_basic(ctx, i) ); > > Or just to permit > rc = write_one_vcpu_basic(ctx, i); if (rc) goto error; > (ie on a single line). OTOH, it should come as no surprise that I'd rather drop this patch entirely than go with these alternatives, both of which detract from code clarity. The former for hiding control flow, and the latter for being atypical layout which unnecessary cognitive load to follow. ~Andrew
Andrew Cooper writes ("Re: [PATCH 01/12] libxc/save: Shrink code volume where possible"):
> On 14/01/2020 16:48, Ian Jackson wrote:
> > Andrew Cooper writes ("[PATCH 01/12] libxc/save: Shrink code volume where possible"):
> >> A property of how the error handling (0 on success, nonzero otherwise)
> >> allows these calls to be chained together with the ternary operatior.
> > I'm quite surprised to find a suggestion like this coming from you in
> > particular.
>
> What probably is relevant is that ?: is a common construct in the
> hypervisor, which I suppose does colour my expectation of people knowing
> exactly what it means and how it behaves.
I expect other C programmers to know what ?: does, too. But I think
using it to implement the error monad is quite unusual. I asked
around a bit and my feeling is still that this isn't an improvement.
> > Or just to permit
> > rc = write_one_vcpu_basic(ctx, i); if (rc) goto error;
> > (ie on a single line).
>
> OTOH, it should come as no surprise that I'd rather drop this patch
> entirely than go with these alternatives, both of which detract from
> code clarity. The former for hiding control flow, and the latter for
> being atypical layout which unnecessary cognitive load to follow.
I think, then, that it would be best to drop this patch, unless Wei
(or someone else) disagrees with me.
Sorry,
Ian.
On Mon, Apr 27, 2020 at 06:19:37PM +0100, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH 01/12] libxc/save: Shrink code volume where possible"): > > On 14/01/2020 16:48, Ian Jackson wrote: > > > Andrew Cooper writes ("[PATCH 01/12] libxc/save: Shrink code volume where possible"): > > >> A property of how the error handling (0 on success, nonzero otherwise) > > >> allows these calls to be chained together with the ternary operatior. > > > I'm quite surprised to find a suggestion like this coming from you in > > > particular. > > > > What probably is relevant is that ?: is a common construct in the > > hypervisor, which I suppose does colour my expectation of people knowing > > exactly what it means and how it behaves. > > I expect other C programmers to know what ?: does, too. But I think > using it to implement the error monad is quite unusual. I asked > around a bit and my feeling is still that this isn't an improvement. > > > > Or just to permit > > > rc = write_one_vcpu_basic(ctx, i); if (rc) goto error; > > > (ie on a single line). > > > > OTOH, it should come as no surprise that I'd rather drop this patch > > entirely than go with these alternatives, both of which detract from > > code clarity. The former for hiding control flow, and the latter for > > being atypical layout which unnecessary cognitive load to follow. > > I think, then, that it would be best to drop this patch, unless Wei > (or someone else) disagrees with me. I don't feel strongly either way. Wei.
On 27/04/2020 20:55, Wei Liu wrote: > On Mon, Apr 27, 2020 at 06:19:37PM +0100, Ian Jackson wrote: >> Andrew Cooper writes ("Re: [PATCH 01/12] libxc/save: Shrink code volume where possible"): >>> On 14/01/2020 16:48, Ian Jackson wrote: >>>> Andrew Cooper writes ("[PATCH 01/12] libxc/save: Shrink code volume where possible"): >>>>> A property of how the error handling (0 on success, nonzero otherwise) >>>>> allows these calls to be chained together with the ternary operatior. >>>> I'm quite surprised to find a suggestion like this coming from you in >>>> particular. >>> What probably is relevant is that ?: is a common construct in the >>> hypervisor, which I suppose does colour my expectation of people knowing >>> exactly what it means and how it behaves. >> I expect other C programmers to know what ?: does, too. But I think >> using it to implement the error monad is quite unusual. I asked >> around a bit and my feeling is still that this isn't an improvement. >> >>>> Or just to permit >>>> rc = write_one_vcpu_basic(ctx, i); if (rc) goto error; >>>> (ie on a single line). >>> OTOH, it should come as no surprise that I'd rather drop this patch >>> entirely than go with these alternatives, both of which detract from >>> code clarity. The former for hiding control flow, and the latter for >>> being atypical layout which unnecessary cognitive load to follow. >> I think, then, that it would be best to drop this patch, unless Wei >> (or someone else) disagrees with me. > I don't feel strongly either way. I'm confused... I dropped this 3 and a half months ago, because it was blindingly obvious it was going nowhere. This is the v1 series which was totally superseded by the v2 series also posted in January. ~Andrew
On Mon, Apr 27, 2020 at 09:00:30PM +0100, Andrew Cooper wrote: > On 27/04/2020 20:55, Wei Liu wrote: > > On Mon, Apr 27, 2020 at 06:19:37PM +0100, Ian Jackson wrote: > >> Andrew Cooper writes ("Re: [PATCH 01/12] libxc/save: Shrink code volume where possible"): > >>> On 14/01/2020 16:48, Ian Jackson wrote: > >>>> Andrew Cooper writes ("[PATCH 01/12] libxc/save: Shrink code volume where possible"): > >>>>> A property of how the error handling (0 on success, nonzero otherwise) > >>>>> allows these calls to be chained together with the ternary operatior. > >>>> I'm quite surprised to find a suggestion like this coming from you in > >>>> particular. > >>> What probably is relevant is that ?: is a common construct in the > >>> hypervisor, which I suppose does colour my expectation of people knowing > >>> exactly what it means and how it behaves. > >> I expect other C programmers to know what ?: does, too. But I think > >> using it to implement the error monad is quite unusual. I asked > >> around a bit and my feeling is still that this isn't an improvement. > >> > >>>> Or just to permit > >>>> rc = write_one_vcpu_basic(ctx, i); if (rc) goto error; > >>>> (ie on a single line). > >>> OTOH, it should come as no surprise that I'd rather drop this patch > >>> entirely than go with these alternatives, both of which detract from > >>> code clarity. The former for hiding control flow, and the latter for > >>> being atypical layout which unnecessary cognitive load to follow. > >> I think, then, that it would be best to drop this patch, unless Wei > >> (or someone else) disagrees with me. > > I don't feel strongly either way. > > I'm confused... I dropped this 3 and a half months ago, because it was > blindingly obvious it was going nowhere. > > This is the v1 series which was totally superseded by the v2 series also > posted in January. OK. I saw Ian's reply on 27th and thought it was still in progress. Wei.
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c index f89e12c99f..9764aa743f 100644 --- a/tools/libxc/xc_sr_save.c +++ b/tools/libxc/xc_sr_save.c @@ -845,11 +845,8 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type) xc_report_progress_single(xch, "Start of stream"); - rc = write_headers(ctx, guest_type); - if ( rc ) - goto err; - - rc = ctx->save.ops.start_of_stream(ctx); + rc = (write_headers(ctx, guest_type) ?: + ctx->save.ops.start_of_stream(ctx)); if ( rc ) goto err; diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c index 3d86cb0600..d925a81999 100644 --- a/tools/libxc/xc_sr_save_x86_hvm.c +++ b/tools/libxc/xc_sr_save_x86_hvm.c @@ -187,24 +187,9 @@ static int x86_hvm_check_vm_state(struct xc_sr_context *ctx) static int x86_hvm_end_of_checkpoint(struct xc_sr_context *ctx) { - int rc; - - /* Write the TSC record. */ - rc = write_x86_tsc_info(ctx); - if ( rc ) - return rc; - - /* Write the HVM_CONTEXT record. */ - rc = write_hvm_context(ctx); - if ( rc ) - return rc; - - /* Write HVM_PARAMS record contains applicable HVM params. */ - rc = write_hvm_params(ctx); - if ( rc ) - return rc; - - return 0; + return (write_x86_tsc_info(ctx) ?: + write_hvm_context(ctx) ?: + write_hvm_params(ctx)); } static int x86_hvm_cleanup(struct xc_sr_context *ctx) diff --git a/tools/libxc/xc_sr_save_x86_pv.c b/tools/libxc/xc_sr_save_x86_pv.c index 3ebc5a2bf8..94d0f68911 100644 --- a/tools/libxc/xc_sr_save_x86_pv.c +++ b/tools/libxc/xc_sr_save_x86_pv.c @@ -768,19 +768,10 @@ static int write_all_vcpu_information(struct xc_sr_context *ctx) if ( !vinfo.online ) continue; - rc = write_one_vcpu_basic(ctx, i); - if ( rc ) - return rc; - - rc = write_one_vcpu_extended(ctx, i); - if ( rc ) - return rc; - - rc = write_one_vcpu_xsave(ctx, i); - if ( rc ) - return rc; - - rc = write_one_vcpu_msrs(ctx, i); + rc = (write_one_vcpu_basic(ctx, i) ?: + write_one_vcpu_extended(ctx, i) ?: + write_one_vcpu_xsave(ctx, i) ?: + write_one_vcpu_msrs(ctx, i)); if ( rc ) return rc; } @@ -1031,25 +1022,10 @@ static int x86_pv_normalise_page(struct xc_sr_context *ctx, xen_pfn_t type, */ static int x86_pv_setup(struct xc_sr_context *ctx) { - int rc; - - rc = x86_pv_domain_info(ctx); - if ( rc ) - return rc; - - rc = x86_pv_map_m2p(ctx); - if ( rc ) - return rc; - - rc = map_shinfo(ctx); - if ( rc ) - return rc; - - rc = map_p2m(ctx); - if ( rc ) - return rc; - - return 0; + return (x86_pv_domain_info(ctx) ?: + x86_pv_map_m2p(ctx) ?: + map_shinfo(ctx) ?: + map_p2m(ctx)); } static int x86_pv_start_of_stream(struct xc_sr_context *ctx) @@ -1080,21 +1056,9 @@ static int x86_pv_start_of_checkpoint(struct xc_sr_context *ctx) static int x86_pv_end_of_checkpoint(struct xc_sr_context *ctx) { - int rc; - - rc = write_x86_tsc_info(ctx); - if ( rc ) - return rc; - - rc = write_shared_info(ctx); - if ( rc ) - return rc; - - rc = write_all_vcpu_information(ctx); - if ( rc ) - return rc; - - return 0; + return (write_x86_tsc_info(ctx) ?: + write_shared_info(ctx) ?: + write_all_vcpu_information(ctx)); } static int x86_pv_check_vm_state(struct xc_sr_context *ctx)
A property of how the error handling (0 on success, nonzero otherwise) allows these calls to be chained together with the ternary operatior. No functional change, but far less boilerplate code. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Jackson <Ian.Jackson@citrix.com> CC: Wei Liu <wl@xen.org> --- tools/libxc/xc_sr_save.c | 7 ++--- tools/libxc/xc_sr_save_x86_hvm.c | 21 +++------------ tools/libxc/xc_sr_save_x86_pv.c | 58 ++++++++-------------------------------- 3 files changed, 16 insertions(+), 70 deletions(-)