Re: [Xen-devel] [PATCH 01/12] libxc/save: Shrink code volume where possible

2020-01-15 Thread Andrew Cooper 
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

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 01/12] libxc/save: Shrink code volume where possible

2020-01-14 Thread Ian Jackson 
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.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 01/12] libxc/save: Shrink code volume where possible

2020-01-14 Thread Ian Jackson 
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.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 01/12] libxc/save: Shrink code volume where possible

2019-12-24 Thread Andrew Cooper 
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 
---
CC: Ian Jackson 
CC: Wei Liu 
---
 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(-)

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)
-- 
2.11.0


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel