Re: [Xen-devel] [PATCH v4 3/4] libxl: call hotplug scripts from libxl for vbd

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[Xen-devel] [PATCH v4 3/4] libxl: call hotplug scripts 
from libxl for vbd"):
> This is a rather big change, that adds the necessary machinery to
> perform hotplug script calling from libxl for Linux. This patch
> launches the necessary scripts to attach and detach PHY and TAP
> backend types (Qemu doesn't use hotplug scripts).

Thanks.

> -SUBSYSTEM=="xen-backend", KERNEL=="tap*", RUN+="/etc/xen/scripts/blktap 
> $env{ACTION}"
> -SUBSYSTEM=="xen-backend", KERNEL=="vbd*", RUN+="/etc/xen/scripts/block 
> $env{ACTION}"
> +SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{UDEV_CALL}="1", 
> RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> +SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{UDEV_CALL}="1", 
> RUN+="/etc/xen/scripts/block $env{ACTION}"

Wouldn't it be better to have an env var set by libxl to mean "_not_
called from udev?".  That would mean that if the udev rules weren't
updated for some reason it would all still work.  On many setups these
are config files which may require the user to merge changes, etc., so
this is a real concern.

> +# Hack to prevent the execution of hotplug scripts from udev if the domain
> +# has been launched from libxl
> +if [ -n "${UDEV_CALL}" ] && \
> +   `xenstore-read "libxl/disable_udev" >/dev/null 2>&1`; then

This reads something from xenstore and executes it as a shell command!
(Also it will go wrong if the value read is empty eg becaue the key
doesn't exist.)

Also didn't I say that in xenstore we normally record booleans as
decimal integers, "0" for false and "1" for true ?

> -}    
> +}
>  
> -int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
> -{
> -    GC_INIT(ctx);
> -    char *dom_path;
> -    char *vm_path;
> -    char *pid;
> -    int rc, dm_present;
> +/* Callback for domain destruction */

The rest of this is very very difficult to review because of the
amount of code motion, variable renaming, and so forth.

Do you think it would be possible to reorganise things so that the
diff is as minimal as possible ?  Techniques you will probably find
useful include:

 * Declare some convenience variables such as
      uint32_t const domid = dis->domid;
   This is best done near the top of each function along with the new
   function prototype etc., so it doesn't introduce a mid-function
   patch hunk.  This will avoid irrelevant noise in the diff caused by
   the movement of state into the operation state struct.

 * Declare callback functions at the top of the file so that they can
   appear after the point where they are referenced, and write all the
   code in the order in which it actually occurs.  This is useful
   anyway, but it probably means that the code won't need to move
   because it's probably already in that order.  If it isn't you may
   need to move it about separately.

 * In general, avoid moving any code if at all possible in the main
   patch.  If you need to move code, do it in a pre- or post-patch.

 * Only take the opportunity to make unrelated changes (eg, changing
   from libxl__sprintf to GCSPRINTF) if you have to make another
   change to the very same line of code.

 * If any code motion is needed, split it out into a pre-patch which
   contains only code motion.

 * Be prepared to leave wrinkles in the code style or layout and fix
   them up in a later patch.

 * If it is necessary to switch a variable from a "struct foo" to a
   "struct foo*", split out a separate pre-patch which changes it to a
   "struct foo[1]".  This separate patch will then obviously have no
   functional change and wiull simply contain a lot of removing of "&"
   etc.  The actual patch with the meat can leave all those references
   unchanged.

 * Likewise if you want to change something from a "struct foo*" to a
   "struct foo" you can do the reverse: in your main patch change it
   to "struct foo[1]" instead.  Then, you can add a post-patch to fix
   that up by changing "struct foo[1]" to "struct foo" which again
   will have no functional change.

You will see me applying these techniques in my recent child process
series; I can point you at specific commits etc. if you want more
hints.

Of course this may not be feasible.  If for some parts of the code
this approach is not feasible, and the result is always going to look
like a rewrite, mention in the commit message that such-and-such
function(s) or parts of code have essentially been rewritten.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel