Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial

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 <console /> (and sometimes <serial /> 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: >>> >>> <console type='pty'> >>> <target type='xen' port='0'/> >>> </console> >>> >>> Which then on domain start gets filled like: >>> >>> <console type='pty' tty='/dev/pts/3'> >>> <source path='/dev/pts/3'/> >>> <target type='xen' port='0'/> >>> </console> >>> >>> Although for HVM guests we have: >>> >>> <serial type='pty'> >>> <target port='0'/> >>> </serial> >>> <console type='pty'> >>> <target type='serial' port='0'/> >>> </console> >>> >>> 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: >>> >>> <serial type='pty'> >>> <source path='/dev/pts/4'/> >>> <target port='0'/> >>> </serial> >>> <console type='pty' tty='/dev/pts/4'> >>> <source path='/dev/pts/4'/> >>> <target type='serial' port='0'/> >>> </console> >> 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 <null> is not using a PTY >> >> My config only contained >> >> <console type='pty'> >> <target port='0'/> >> </console> >> >> 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 <console type='pty'> <target port='0'/> </console> After define <serial type='pty'> <target port='0'/> </serial> <console type='pty'> <target type='serial' port='0'/> </console> 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?).

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 <jfehlig(a)suse.com&gt; >> Date: Tue, 21 Jun 2016 20:25:23 -0600 >> Subject: [PATCH] domain conf: copy source type from stolen console >> >> When domXML contains only <console type='pty'> and no corresponding >> <serial>, the console is "stolen" [1] and used as the first <serial> >> device. A new console is created as an alias to the first <serial> >> 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 <jfehlig(a)suse.com&gt; >> --- >> 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 >