From patchwork Wed Jun 22 18:45:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cole Robinson X-Patchwork-Id: 70680 Delivered-To: patch@linaro.org Received: by 10.140.28.4 with SMTP id 4csp55905qgy; Wed, 22 Jun 2016 11:48:08 -0700 (PDT) X-Received: by 10.28.132.15 with SMTP id g15mr10002718wmd.67.1466621288051; Wed, 22 Jun 2016 11:48:08 -0700 (PDT) Return-Path: Received: from mx4-phx2.redhat.com (mx4-phx2.redhat.com. [209.132.183.25]) by mx.google.com with ESMTPS id m204si227907wmd.45.2016.06.22.11.48.07 (version=TLS1 cipher=AES128-SHA bits=128/128); Wed, 22 Jun 2016 11:48:08 -0700 (PDT) Received-SPF: pass (google.com: domain of libvir-list-bounces@redhat.com designates 209.132.183.25 as permitted sender) client-ip=209.132.183.25; Authentication-Results: mx.google.com; spf=pass (google.com: domain of libvir-list-bounces@redhat.com designates 209.132.183.25 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx4-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u5MIjFAb007995; Wed, 22 Jun 2016 14:45:16 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u5MIjEIA032316 for ; Wed, 22 Jun 2016 14:45:14 -0400 Received: from [10.10.116.65] (ovpn-116-65.rdu2.redhat.com [10.10.116.65]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5MIjDo1030380; Wed, 22 Jun 2016 14:45:13 -0400 To: Jim Fehlig , Joao Martins References: <1466447465-25137-1-git-send-email-joao.m.martins@oracle.com> <57688C68.3060606@suse.com> <576914F9.2070104@oracle.com> <5769F8BD.5030908@suse.com> <94455b60-ce38-5635-e197-a7c344d556c1@redhat.com> <576AC0BB.6050503@suse.com> From: Cole Robinson Message-ID: <6a935ece-4c45-d65d-5a87-94328485becf@redhat.com> Date: Wed, 22 Jun 2016 14:45:13 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <576AC0BB.6050503@suse.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-loop: libvir-list@redhat.com Cc: libvir-list@redhat.com, Peter Krempa Subject: Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com On 06/22/2016 12:45 PM, Jim Fehlig wrote: > On 06/22/2016 06:07 AM, Cole Robinson wrote: >> On 06/21/2016 10:32 PM, Jim Fehlig wrote: >>> On 06/21/2016 04:20 AM, Joao Martins wrote: >>>> On 06/21/2016 01:38 AM, Jim Fehlig wrote: >>>>> Joao Martins wrote: >>>>>> Guests use a (and sometimes pair) to represent >>>>>> the console. On the callback that is called when console is brought up >>>>>> (NB: before domain starts), we fill the path of the console element with >>>>>> the appropriate "/dev/pts/X". For PV guests it all works fine, although >>>>>> for HVM guests it doesn't. Consequently we end up seeing erronous >>>>>> behaviour on HVM guests where console path is not displayed on the XML >>>>>> but still can be accessed with virDomainOpenConsole after booting guest. >>>>>> Consumers of this XML (e.g. Openstack) also won't be able to use the >>>>>> console path. >>>>> Can you provide example input domXML config that causes this problem? >>>>> >>>> Ah yes - I probably should include that in the commit description? >>> Yes, I think it helps clarify the problem being solved by the commit. >>> >>>> So, for PV guests for an input console XML element: >>>> >>>> >>>> >>>> >>>> >>>> Which then on domain start gets filled like: >>>> >>>> >>>> >>>> >>>> >>>> >>>> Although for HVM guests we have: >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> And no changes happen i.e. source path isn't written there _even though_ it's >>>> set on the console element - being the reason why we can access the console in the >>>> first place. IIUC The expected output should be: >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>> I asked for an example after having problems testing your patch, but it turned >>> out to be an issue which I think was introduced long ago by commit 482e5f15. I >>> was testing with transient domains and noticed 'virsh create; 'virsh console' >>> always failed with >>> >>> error: internal error: character device is not using a PTY >>> >>> My config only contained >>> >>> >>> >>> >>> >>> so the "really crazy backcompat stuff for consoles" in >>> virDomainDefAddConsoleCompat() moved the config over to def->serials[0] and >>> created a new def->consoles[0] without def->consoles[0].source.type set to >>> VIR_DOMAIN_CHR_TYPE_PTY. Without source.type set, libxlConsoleCallback() would >>> ignore the console and not copy the PTY path to source.data.file.path. 'virsh >>> console' would then fail with the error noted above. >>> >>> I spent too much time debugging this, partly because I was confused by odd >>> behavior with persistent domains. E.g. 'virsh define; virsh start; virsh >>> console' would fail with the same error, but all subsequent 'virsh shutdown; >>> virsh start; virsh console' sequences would work :-). That behavior was due to >>> vm->newDef being populated with correct console config when calling >>> virDomainObjSetDefTransient() in libxlDomainStart(). After the first shutdown of >>> the domain, vm->newDef would be copied over to vm->def, allowing subsequent >>> 'virsh start; virsh console' to work correctly. >>> >> Hmm, that sounds weird. Can you explain more how exactly the XML changes? > > It only changes when defining the vm. > >> What >> is the diff between the initial 'virsh define; virsh dumpxml' > > Initial XML has > > > > > > After define > > > > > > > > > The config doesn't change after start; shutdown. But the contents of > virDomainDef does change, specifically def->consoles[0]->source.type changes > from VIR_DOMAIN_CHR_TYPE_PTY to VIR_DOMAIN_CHR_TYPE_NULL. And as mentioned > below, libxlConsoleCallback() only checks for source.type == > VIR_DOMAIN_CHR_TYPE_PTY before copying the PTY path to > def->consoles[0]->source.data.file.path. Another potential fix I also mentioned > below is to loosen that condition in libxlConsoleCallback(). I.e. copy the path > if source.type equals VIR_DOMAIN_CHR_TYPE_PTY or VIR_DOMAIN_CHR_TYPE_NULL (and > perhaps VIR_DOMAIN_CHR_TYPE_FILE too?). > Okay I think I follow. The thing I was missing is that virDomainDefAddConsoleCompat will alter ->def, but that's _not_ what is formated/printed in the XML, which has some special handling to fully duplicate serials[0] XML to consoles[0], if console target type=serial I _think_ what the proper thing to do here is something like: chr->target.port, console_type, Untested, but basically, libxl driver needs to learn that if consoles[0].target.type == SERIAL, then you should use serials[0] instead. The qemu driver has some magic like this sprinkled around... The problem with your patch below is that it's not a whole fix, there's many other bits besides source.type that would technically need to be copied over. Rather than doing that, which has its own problems trying to keep the console[0] and serial[0] data in sync, the convention seems to be that console gets only target.type=serial and the drivers need to map that to serials[0]. That's my reading of things anyways Thanks, Cole > Regards, > Jim > >> and the dumpxml >> after the VM has started+shutdown once? Whatever that diff is, and why it's >> not in the XML to begin with, sounds like the root issue to me. >> >>> Long story short, I found the problem but not sure how to fix it :-). One >>> approach would be the below patch. Another would be to loosen the restriction in >>> libxlConsoleCallback, filing in source.data.file.path when the console source >>> type is also set to VIR_DOMAIN_CHR_TYPE_NULL. Adding Cole and Peter (both of >>> which have toiled in and likely loathe this code) to see if they have opinions >>> on a proper fix. >>> >>> Regards, >>> Jim >>> >>> >>> From a231636e8eec08152220e32af5f4026a50954f41 Mon Sep 17 00:00:00 2001 >>> From: Jim Fehlig >>> Date: Tue, 21 Jun 2016 20:25:23 -0600 >>> Subject: [PATCH] domain conf: copy source type from stolen console >>> >>> When domXML contains only and no corresponding >>> , the console is "stolen" [1] and used as the first >>> device. A new console is created as an alias to the first >>> device, but missed copying some configuration such as the source >>> 'type' attribute. >>> >>> [1] See comments associated with virDomainDefAddConsoleCompat() in >>> $LIBVIRT-SRC/src/conf/domain_conf.c: >>> >>> Signed-off-by: Jim Fehlig >>> --- >>> src/conf/domain_conf.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>> index 75ad03f..2fda4fe 100644 >>> --- a/src/conf/domain_conf.c >>> +++ b/src/conf/domain_conf.c >>> @@ -3845,6 +3845,7 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def) >>> /* Create an console alias for the serial port */ >>> def->consoles[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; >>> def->consoles[0]->targetType = >>> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL; >>> + def->consoles[0]->source.type = def->serials[0]->source.type; >>> } >>> } else if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && def->nserials > 0 && >>> def->serials[0]->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && >>> >> Then again this also seems okay to me, but it's concerning that I applied this >> but it didn't cause the test suite to change at all. >> >> - Cole >> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 221af87..6bcb4d9 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -964,13 +964,17 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback) virObjectLock(vm); for (i = 0; i < vm->def->nconsoles; i++) { virDomainChrDefPtr chr = vm->def->consoles[i]; - if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + if (i == 0 && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) + chr = vm->def->serials[0]; + + if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { libxl_console_type console_type; char *console = NULL; int ret; console_type = - (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL ? LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); ret = libxl_console_get_tty(ctx, ev->domid,