| Message ID | 20200103130616.13724-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Andrew Cooper writes ("[PATCH 16/20] tools/libxl: Simplify callback handling in libxl-save-helper"):
> The {save,restore}_callback helpers can have their scope reduced vastly,
This part is OK with me although it would have been nicer to review if
the the move and the rename were separate patches. I don't know why
it is valuable.
> and helper_setcallbacks_{save,restore}() doesn't need to use a
> ternary operator to write 0 (meaning NULL) into an already zeroed
> structure.
Is this unrelated ? I think so.
> my $c_cb = "cbs->$name";
> $f_more_sr->(" if ($c_cb) cbflags |= $c_v;\n", $enumcallbacks);
> - $f_more_sr->(" $c_cb = (cbflags & $c_v) ? ${encode}_${name} : 0;\n",
> + $f_more_sr->(" if (cbflags & $c_v) $c_cb = ${encode}_${name};\n",
> $setcallbacks);
It is a long time since I edited this code but I think your reasoning
is "cbs is already zero on entry because it is static; therefore
cbs->$name must be null, so there is no need to write 0 into it in the
else case".
However, the line you are touching is preceded by "if ($c_cb)" which
only makes sense if the variable might be non-null.
So something is not right here.
Ian.
On 14/01/2020 17:27, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH 16/20] tools/libxl: Simplify callback handling in libxl-save-helper"): >> The {save,restore}_callback helpers can have their scope reduced vastly, > This part is OK with me although it would have been nicer to review if > the the move and the rename were separate patches. I don't know why > it is valuable. > >> and helper_setcallbacks_{save,restore}() doesn't need to use a >> ternary operator to write 0 (meaning NULL) into an already zeroed >> structure. > Is this unrelated ? I think so. This change is specifically to make the generated C easier to follow, because I had to debug it yet again. >> my $c_cb = "cbs->$name"; >> $f_more_sr->(" if ($c_cb) cbflags |= $c_v;\n", $enumcallbacks); >> - $f_more_sr->(" $c_cb = (cbflags & $c_v) ? ${encode}_${name} : 0;\n", >> + $f_more_sr->(" if (cbflags & $c_v) $c_cb = ${encode}_${name};\n", >> $setcallbacks); > It is a long time since I edited this code but I think your reasoning > is "cbs is already zero on entry because it is static; therefore > cbs->$name must be null, so there is no need to write 0 into it in the > else case". Correct. > > However, the line you are touching is preceded by "if ($c_cb)" which > only makes sense if the variable might be non-null. > > So something is not right here. This is all perl to me, but the two adjacent-looking lines of C don't end up adjacent in the generated code. The first line ends up in libxl__srm_callout_enumcallbacks_{save,restore}() (in libxl.so), while the second line ends up in helper_setcallbacks_{save,restore}() (in libxl-save-helper). ~Andrew
diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c index 398df00dd6..a91f36db73 100644 --- a/tools/libxl/libxl_save_helper.c +++ b/tools/libxl/libxl_save_helper.c @@ -217,8 +217,6 @@ int helper_getreply(void *user) /*----- other callbacks -----*/ -static struct save_callbacks helper_save_callbacks; - static void startup(const char *op) { xtl_log(&logger,XTL_DEBUG,0,program,"starting %s",op); @@ -234,8 +232,6 @@ static void complete(int retval) { exit(0); } -static struct restore_callbacks helper_restore_callbacks; - int main(int argc, char **argv) { int r; @@ -247,6 +243,7 @@ int main(int argc, char **argv) assert(mode); if (!strcmp(mode,"--save-domain")) { + static struct save_callbacks cb; io_fd = atoi(NEXTARG); recv_fd = atoi(NEXTARG); @@ -257,16 +254,17 @@ int main(int argc, char **argv) xc_stream_type_t stream_type = strtoul(NEXTARG,0,10); assert(!*++argv); - helper_setcallbacks_save(&helper_save_callbacks, cbflags); + helper_setcallbacks_save(&cb, cbflags); startup("save"); setup_signals(save_signal_handler); - r = xc_domain_save(xch, io_fd, dom, flags, &helper_save_callbacks, + r = xc_domain_save(xch, io_fd, dom, flags, &cb, hvm, stream_type, recv_fd); complete(r); } else if (!strcmp(mode,"--restore-domain")) { + static struct restore_callbacks cb; io_fd = atoi(NEXTARG); send_back_fd = atoi(NEXTARG); @@ -281,7 +279,7 @@ int main(int argc, char **argv) xc_stream_type_t stream_type = strtoul(NEXTARG,0,10); assert(!*++argv); - helper_setcallbacks_restore(&helper_restore_callbacks, cbflags); + helper_setcallbacks_restore(&cb, cbflags); unsigned long store_mfn = 0; unsigned long console_mfn = 0; @@ -292,8 +290,7 @@ int main(int argc, char **argv) r = xc_domain_restore(xch, io_fd, dom, store_evtchn, &store_mfn, store_domid, console_evtchn, &console_mfn, console_domid, hvm, pae, - stream_type, - &helper_restore_callbacks, send_back_fd); + stream_type, &cb, send_back_fd); helper_stub_restore_results(store_mfn,console_mfn,0); complete(r); diff --git a/tools/libxl/libxl_save_msgs_gen.pl b/tools/libxl/libxl_save_msgs_gen.pl index 6f1d79f821..831a15e0bb 100755 --- a/tools/libxl/libxl_save_msgs_gen.pl +++ b/tools/libxl/libxl_save_msgs_gen.pl @@ -333,7 +333,7 @@ END_ALWAYS my $c_v = "(1u<<$msgnum)"; my $c_cb = "cbs->$name"; $f_more_sr->(" if ($c_cb) cbflags |= $c_v;\n", $enumcallbacks); - $f_more_sr->(" $c_cb = (cbflags & $c_v) ? ${encode}_${name} : 0;\n", + $f_more_sr->(" if (cbflags & $c_v) $c_cb = ${encode}_${name};\n", $setcallbacks); } $f_more_sr->(" return 1;\n }\n\n");
The {save,restore}_callback helpers can have their scope reduced vastly, and helper_setcallbacks_{save,restore}() doesn't need to use a ternary operator to write 0 (meaning NULL) into an already zeroed structure. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Jackson <Ian.Jackson@citrix.com> CC: Wei Liu <wl@xen.org> CC: Anthony PERARD <anthony.perard@citrix.com> --- tools/libxl/libxl_save_helper.c | 15 ++++++--------- tools/libxl/libxl_save_msgs_gen.pl | 2 +- 2 files changed, 7 insertions(+), 10 deletions(-)