From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device Date: Wed, 30 May 2012 09:43:59 +0100 Message-ID: <4FC5DDCF.5050008@citrix.com> References: <1337855045-10428-1-git-send-email-roger.pau@citrix.com> <1337855045-10428-2-git-send-email-roger.pau@citrix.com> <20420.53243.895944.536517@mariner.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: <20420.53243.895944.536517@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 wrote: > Roger Pau Monne writes ("[PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device"): >> Introduce a new structure to track state of device backends, that will >> be used in following patches on this series. >> >> This structure if used for both device creation and device >> destruction and removes libxl__ao_device_remove. > ... >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index e3d05c2..7fdecf1 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c > ... >> +/* Macro for defining device remove/destroy functions in a compact way */ >> +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \ > > Looks reasonable to me. > >> +/* Define all remove/destroy functions and undef the macro */ >> + >> +/* disk */ >> +DEFINE_DEVICE_REMOVE(disk, remove, 0) >> +DEFINE_DEVICE_REMOVE(disk, destroy, 1) > > I'm not sure what purpose the comment serves but I don't really object > to it :-). > >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index 52f5435..670a17b 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h > ... >> @@ -830,13 +830,6 @@ _hidden const char *libxl__device_nic_devname(libxl__gc * >> +/* This functions sets the necessary libxl__ao_device struct values to use >> + * safely inside functions. It marks the operation as "active" >> + * since we need to be sure that all device status structs are set >> + * to active before start queueing events, or we might call >> + * ao_complete before all devices had finished */ >> +_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao, >> + libxl__ao_device **base); > > You need to clearly explain what the constraints are on the order of > calling _prepare and _initiate_..._remove. > > Questions whose answers should be clear from your text include: > - is it legal to call _initiate_ without having previously called > _prepare ? > - is it legal to call _prepare and then simply throw away the > aodev ? > - is it legal to call _prepare multiple times ? (If the answer > to the previous question is `yes' then it must be, I think.) I've added a more detailed comment about _prepare. >> @@ -436,34 +441,35 @@ out: >> -int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao, >> - libxl__device *dev) >> +void libxl__initiate_device_remove(libxl__egc *egc, >> + libxl__ao_device *aodev) >> { > ... >> +retry_transaction: >> + t = xs_transaction_start(ctx->xsh); >> + if (aodev->force) >> + libxl__xs_path_cleanup(gc, t, >> + libxl__device_frontend_path(gc, aodev->dev)); >> + state = libxl__xs_read(gc, t, state_path); > > Didn't I previously comment adversely on having this check here ? > > Ah yes, I commented on the corresponding code in add (v2 08/15): > > [...], I think all of this is done for you by > libxl__ev_devstate_wait. You don't need to pre-check the state path > at all. > > And: > > Do you really need to do the xenstore state read here ? Surely > libxl__ev_devstate_wait will do it for you. Done. >> + /* Some devices (vkbd) fail to disconnect properly, >> + * but we shouldn't alarm the user if it's during >> + * domain destruction. >> + */ >> + if (rc&& aodev->action == DEVICE_CONNECT) { >> + LOG(ERROR, "unable to connect device with path %s", >> + libxl__device_backend_path(gc, aodev->dev)); >> + goto out; >> + } else if(rc) { > > Missing space after `if'. Done. >> + LOG(DEBUG, "unable to disconnect device with path %s", >> + libxl__device_backend_path(gc, aodev->dev)); >> + goto out; >> + } > > Last time I wrote: > > Perhaps we should have a separate flag which controls error reporting > so that a user-requested graceful device disconnect gets full error > logging ? > > Did you miss the fact that email suggesting the macro for generating > the device remove functions also had these other comments in ? I've replied to this email, the response is here: http://marc.info/?l=xen-devel&m=133770109429587&w=2 I would like to leave this for future work, since the error checking we have now is not better that what I'm proposing here.