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

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

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).


-SUBSYSTEM=="xen-backend", KERNEL=="tap*", RUN+="/etc/xen/scripts/blktap 
-SUBSYSTEM=="xen-backend", KERNEL=="vbd*", RUN+="/etc/xen/scripts/block 
+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

  * 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

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.


Xen-devel mailing list



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