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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

Ian Jackson escribió:
> 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.
Since the 4.2 release already implies updating udev rules (due to the 
change from tap* to *emu), I think it's better to set the var on udev, 
that way when udev is dropped we won't need to perform any change to libxl.
> > +# 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.)
Are you sure about this? This command never returns anything, because it 
is redirected to /dev/null, so we only evaluate if it is able to read 
libxl/disable_udev. If libxl/disable_udev exists this test is passed.
> 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.
Yes, will try to do so. I'm currently moving the changes to mach tip and 
also splitting them.
> 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