[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.