From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v11 01/17] libxl: fix stubdom console destruction Date: Tue, 24 Jul 2012 09:39:57 +0100 Message-ID: <500E5F5D.1000601@citrix.com> References: <1343064465-17864-1-git-send-email-roger.pau@citrix.com> <1343064465-17864-2-git-send-email-roger.pau@citrix.com> <1343116236.8016.24.camel@dagon.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1343116236.8016.24.camel@dagon.hellion.org.uk> 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: Stefano Stabellini , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org Ian Campbell wrote: > On Mon, 2012-07-23 at 18:27 +0100, Roger Pau Monne wrote: >> Stubdoms have several consoles attached, and they don't follow the >> xenstore protocol for devices, since they are always in state 1. We >> have to add an exception to libxl__initiate_device_remove, so libxl >> doesn't wait for them to reach state 6 (Closed). >> >> Report: http://markmail.org/message/yqgppcsdip6tnmh6 >> >> Cc: Ian Jackson >> Reported-by: Ian Campbell >> Signed-off-by: Roger Pau Monne >> --- >> tools/libxl/libxl_device.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >> index a94beab..c4392fa 100644 >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -592,8 +592,10 @@ void libxl__initiate_device_remove(libxl__egc *egc, >> LOG(ERROR, "unable to get info for domain %d", domid); >> goto out; >> } >> - if (QEMU_BACKEND(aodev->dev) && >> - (info.paused || info.dying || info.shutdown)) { >> + if ((QEMU_BACKEND(aodev->dev) && >> + (info.paused || info.dying || info.shutdown)) || >> + (libxl_is_stubdom(CTX, aodev->dev->domid, NULL) && >> + (aodev->dev->backend_kind == LIBXL__DEVICE_KIND_CONSOLE))) { > > Is this actually specific to stubdom consoles or is that just where > we've noticed it? It's just that I've noticed it with stubdoms, which AFAIK are the only domains that can have more than one console. > I don't see why it wouldn't be relevant to any PV > console other than the first (which I assume you special case > elsewhere?) Yes, the PV console special case was already there before my changes, I just left it untouched. > Probably the logic would be clearer in a helper, i.e. > dev_is_stubdom_console? or encapsulate both bits of logic in > dev_needs_shutdown? Yes. > Does it apply to LIBXL__CONSOLE_BACKEND_IOEMU as well > as ..._XENCONSOLED? I took a look through tools/console and I cannot > find any handling of a state node in xenstore at all, so the XENCONSOLED > case seems clear. I notice that xen_console.c registers the device with > DEVOPS_FLAG_IGNORE_STATE but that only seems to affect startup not > teardown. I don't see a qemu_chr_close (or anything similar) anywhere in > hw/xen_console.c So it should apply to any console device? This means I don't have to wait for any device of type LIBXL__DEVICE_KIND_CONSOLE, and there's no need to check for the specific console type or Qemu. > >> /* >> * TODO: 4.2 Bodge due to QEMU, see comment on top of >> * libxl__initiate_device_remove in libxl_internal.h > > I suppose this comment needs updating now that the conditional has > changed? In particular the special handling of consoles is not a qemu > related bodge. I thought it was a Qemu related issue, but I will change the comment or do this check in a different "if".