From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v8 10/15] libxl: add option to choose who executes hotplug scripts Date: Tue, 10 Jul 2012 10:20:43 +0100 Message-ID: <4FFBF3EB.1090904@citrix.com> References: <1341403176-50715-1-git-send-email-roger.pau@citrix.com> <1341403176-50715-11-git-send-email-roger.pau@citrix.com> <1341570532.32747.6.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1341570532.32747.6.camel@zakaz.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 Campbell Cc: Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org Ian Campbell wrote: > On Wed, 2012-07-04 at 12:59 +0100, Roger Pau Monne wrote: >> Add and option to xl.conf file to decide if hotplug scripts are >> executed from the toolstack (xl) or from udev as it used to be in the >> past. >> >> This option is only introduced in this patch, but it has no effect >> since the code to call hotplug scripts from libxl is introduced in a >> latter patch. >> >> This choice will be saved in "libxl/disable_udev", as specified in the >> DISABLE_UDEV_PATH constant. >> >> Changes since v2: >> >> * Change atoi(...) to !!atoi(...) to prevent returning negative >> values from xenstore (which will be handled as errors). >> >> * Check for errors on the return value of libxl__hotplug_settings. >> >> Changes since v1: >> >> * Used an auxiliary function to check for the current setting. >> >> Cc: Ian Jackson >> Acked-by: Ian Jackson >> Signed-off-by: Roger Pau Monne >> --- >> docs/man/xl.conf.pod.5 | 8 ++++++++ >> tools/examples/xl.conf | 5 +++++ >> tools/libxl/libxl_create.c | 38 +++++++++++++++++++++++++++++++++++++- >> tools/libxl/libxl_dm.c | 4 ++++ >> tools/libxl/libxl_internal.c | 19 +++++++++++++++++++ >> tools/libxl/libxl_internal.h | 3 +++ >> tools/libxl/libxl_types.idl | 1 + >> tools/libxl/xl.c | 4 ++++ >> tools/libxl/xl.h | 1 + >> tools/libxl/xl_cmdimpl.c | 1 + >> 10 files changed, 83 insertions(+), 1 deletions(-) >> >> diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 >> index 149430c..23932be 100644 >> --- a/docs/man/xl.conf.pod.5 >> +++ b/docs/man/xl.conf.pod.5 >> @@ -55,6 +55,14 @@ default. >> >> Default: C<1> >> >> +=item B >> + >> +If disabled hotplug scripts will be called from udev, as it used to >> +be in the previous releases. With the default option, hotplug scripts >> +will be launched by xl directly. >> + >> +Default: C<1> >> + >> =item B >> >> Sets the path to the lock file used by xl to serialise certain >> diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf >> index ebf057c..28ab796 100644 >> --- a/tools/examples/xl.conf >> +++ b/tools/examples/xl.conf >> @@ -15,3 +15,8 @@ >> >> # first block device to be used for temporary VM disk mounts >> #blkdev_start="xvda" >> + >> +# default option to run hotplug scripts from xl >> +# if disabled the old behaviour will be used, and hotplug scripts will be >> +# launched by udev. >> +#run_hotplug_scripts=1 >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index 6f4c865..c584c4e 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -70,6 +70,8 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc, >> libxl_defbool_setdefault(&c_info->oos, true); >> } >> >> + libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true); >> + >> return 0; >> } >> >> @@ -382,7 +384,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info, >> uint32_t *domid) >> { >> libxl_ctx *ctx = libxl__gc_owner(gc); >> - int flags, ret, rc; >> + int flags, ret, rc, nb_vm; >> char *uuid_string; >> char *dom_path, *vm_path, *libxl_path; >> struct xs_permissions roperm[2]; >> @@ -503,6 +505,40 @@ retry_transaction: >> libxl__sprintf(gc, "%s/hvmloader/generation-id-address", dom_path), >> rwperm, ARRAY_SIZE(rwperm)); >> >> + if (libxl_list_vm(ctx,&nb_vm)< 0) { > > This will leak the returned list of vms... Fixed. > >> + LOG(ERROR, "cannot get number of running guests"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + int hotplug_setting = libxl__hotplug_settings(gc, t); >> + if (hotplug_setting< 0) { >> + LOG(ERROR, "unable to get current hotplug scripts execution setting"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + if (libxl_defbool_val(info->run_hotplug_scripts) != hotplug_setting&& >> + (nb_vm - 1)) { >> + LOG(ERROR, "cannot change hotplug execution option once set, " >> + "please shutdown all guests before changing it"); >> + rc = ERROR_FAIL; >> + goto out; >> + } > >> + >> + if (libxl_defbool_val(info->run_hotplug_scripts)) { >> + rc = libxl__xs_write(gc, t, DISABLE_UDEV_PATH, "1"); >> + if (rc) { >> + LOGE(ERROR, "unable to write %s = 1", DISABLE_UDEV_PATH); >> + goto out; >> + } >> + } else { >> + rc = xs_rm(ctx->xsh, t, DISABLE_UDEV_PATH); >> + if (rc) { >> + LOGE(ERROR, "unable to delete %s", DISABLE_UDEV_PATH); >> + goto out; >> + } >> + } > > You could use libxl__xs_{write,rm}_checked here I think. Yes, when this was written libxl__xs_{write,rm}_checked wasn't there. Now I've changed it, although the rest of the code in this function still use xs_rm or libxl_xs_write. > >> + >> xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/uuid", vm_path), uuid_string, strlen(uuid_string)); >> xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/name", vm_path), info->name, strlen(info->name)); >> >> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c >> index b4a5f1f..612e9db 100644 >> --- a/tools/libxl/libxl_dm.c >> +++ b/tools/libxl/libxl_dm.c >> @@ -762,6 +762,10 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss) >> dm_config->nics = guest_config->nics; >> dm_config->num_nics = guest_config->num_nics; >> >> + libxl_defbool_set( >> +&dm_config->c_info.run_hotplug_scripts, >> + libxl_defbool_val(guest_config->c_info.run_hotplug_scripts)); > > This is the same as: > > dm_config->c_info.run_hotplug_scripts = guest_config->c_info.run_hotplug_scripts; Changed, thanks for the comments.