Re: [Xen-devel] [PATCH 16/20] tools/libxl: Simplify callback handling in libxl-save-helper

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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.

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