From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v4 3/4] libxl: call hotplug scripts from libxl for vbd Date: Mon, 14 May 2012 13:38:22 +0100 Message-ID: <4FB0FCBE.1020802@citrix.com> References: <1336649312-11198-1-git-send-email-roger.pau@citrix.com> <1336649312-11198-4-git-send-email-roger.pau@citrix.com> <20395.60719.885071.452187@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20395.60719.885071.452187@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org Ian Jackson escribi=F3: > Roger Pau Monne writes ("[Xen-devel] [PATCH v4 3/4] libxl: call hotplug s= cripts 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=3D=3D"xen-backend", KERNEL=3D=3D"tap*", RUN+=3D"/etc/xen/scri= pts/blktap $env{ACTION}" >> -SUBSYSTEM=3D=3D"xen-backend", KERNEL=3D=3D"vbd*", RUN+=3D"/etc/xen/scri= pts/block $env{ACTION}" >> +SUBSYSTEM=3D=3D"xen-backend", KERNEL=3D=3D"tap*", ENV{UDEV_CALL}=3D"1",= RUN+=3D"/etc/xen/scripts/blktap $env{ACTION}" >> +SUBSYSTEM=3D=3D"xen-backend", KERNEL=3D=3D"vbd*", ENV{UDEV_CALL}=3D"1",= RUN+=3D"/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 dom= ain >> +# 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 =3D 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.