Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial
On 06/22/2016 12:45 PM, Cole Robinson wrote: > 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 Yes, correct. > > I _think_ what the proper thing to do here is something like: > > > 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) > virObje
Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial
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: 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 && +
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 (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?). 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 >>
Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial
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? What is the diff between the initial 'virsh define; virsh dumpxml' 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 == V
Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial
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. 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 && -- 2.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial
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? 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: Joao > Regards, > Jim > >> Finally, the path set in consoles array won't persist >> across daemon reboots, thus rendering admins/users with no access to >> console with a message such as: >> >> "error: operation failed: PTY device is not yet assigned" >> >> This is because: for HVM guests, DefFormatInternal will ignore the >> console element and instead write it with the content of the serial >> element for target type = serial which isn't touched in our >> libxlConsoleCallback. To address that we introduce a new helper routine >> libxlConsoleSetSourcePath() that sets the source path and thus also >> handling this case. That is by setting the source path on with serial >> element akin to the one indexed by console "port". This way we keep >> similar behaviour for PV and HVM guests. >> >> Signed-off-by: Joao Martins >> --- >> src/libxl/libxl_domain.c | 35 +++ >> 1 file changed, 31 insertions(+), 4 deletions(-) >> >> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c >> index 221af87..4a46143 100644 >> --- a/src/libxl/libxl_domain.c >> +++ b/src/libxl/libxl_domain.c >> @@ -955,6 +955,35 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) >> return 0; >> } >> >> +static int >> +libxlConsoleSetSourcePath(virDomainDefPtr def, virDomainChrDefPtr console, >> + char *path) >> +{ >> +int ret = -1; >> +size_t i; >> + >> +if (!path || path[0] == '\0') >> +return ret; >> + >> +if (VIR_STRDUP(console->source.data.file.path, path) < 0) >> +return ret; >> + >> +if (console->targetType != VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) >> +return 0; >> + >> +for (i = 0; i < def->nserials; i++) { >> +virDomainChrDefPtr serial = def->serials[i]; >> + >> +if (serial->target.port == console->target.port && >> +VIR_STRDUP(serial->source.data.file.path, path) >= 0) { >> +ret = 0; >> +break; >> +} >> +} >> + >> +return ret; >> +} >> + >> static void >> libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback) >> { >> @@ -977,10 +1006,8 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, >> void *for_callback) >> &console); >> if (!ret) { >> VIR_FREE(chr->source.data.file.path); >> -if (console && console[0] != '\0') { >> -ignore_value(VIR_STRDUP(chr->source.data.file.path, >> -console)); >> -} >> +if (libxlConsoleSetSourcePath(vm->def, chr, console) < 0) >> +VIR_WARN("Failed to set console source path"); >> } >> VIR_FREE(console); >> } > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] libxl: set serial source path for console type=serial
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? Regards, Jim > Finally, the path set in consoles array won't persist > across daemon reboots, thus rendering admins/users with no access to > console with a message such as: > > "error: operation failed: PTY device is not yet assigned" > > This is because: for HVM guests, DefFormatInternal will ignore the > console element and instead write it with the content of the serial > element for target type = serial which isn't touched in our > libxlConsoleCallback. To address that we introduce a new helper routine > libxlConsoleSetSourcePath() that sets the source path and thus also > handling this case. That is by setting the source path on with serial > element akin to the one indexed by console "port". This way we keep > similar behaviour for PV and HVM guests. > > Signed-off-by: Joao Martins > --- > src/libxl/libxl_domain.c | 35 +++ > 1 file changed, 31 insertions(+), 4 deletions(-) > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c > index 221af87..4a46143 100644 > --- a/src/libxl/libxl_domain.c > +++ b/src/libxl/libxl_domain.c > @@ -955,6 +955,35 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) > return 0; > } > > +static int > +libxlConsoleSetSourcePath(virDomainDefPtr def, virDomainChrDefPtr console, > + char *path) > +{ > +int ret = -1; > +size_t i; > + > +if (!path || path[0] == '\0') > +return ret; > + > +if (VIR_STRDUP(console->source.data.file.path, path) < 0) > +return ret; > + > +if (console->targetType != VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) > +return 0; > + > +for (i = 0; i < def->nserials; i++) { > +virDomainChrDefPtr serial = def->serials[i]; > + > +if (serial->target.port == console->target.port && > +VIR_STRDUP(serial->source.data.file.path, path) >= 0) { > +ret = 0; > +break; > +} > +} > + > +return ret; > +} > + > static void > libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback) > { > @@ -977,10 +1006,8 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, > void *for_callback) > &console); > if (!ret) { > VIR_FREE(chr->source.data.file.path); > -if (console && console[0] != '\0') { > -ignore_value(VIR_STRDUP(chr->source.data.file.path, > -console)); > -} > +if (libxlConsoleSetSourcePath(vm->def, chr, console) < 0) > +VIR_WARN("Failed to set console source path"); > } > VIR_FREE(console); > } -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RFC] libxl: set serial source path for console type=serial
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. Finally, the path set in consoles array won't persist across daemon reboots, thus rendering admins/users with no access to console with a message such as: "error: operation failed: PTY device is not yet assigned" This is because: for HVM guests, DefFormatInternal will ignore the console element and instead write it with the content of the serial element for target type = serial which isn't touched in our libxlConsoleCallback. To address that we introduce a new helper routine libxlConsoleSetSourcePath() that sets the source path and thus also handling this case. That is by setting the source path on with serial element akin to the one indexed by console "port". This way we keep similar behaviour for PV and HVM guests. Signed-off-by: Joao Martins --- src/libxl/libxl_domain.c | 35 +++ 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 221af87..4a46143 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -955,6 +955,35 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) return 0; } +static int +libxlConsoleSetSourcePath(virDomainDefPtr def, virDomainChrDefPtr console, + char *path) +{ +int ret = -1; +size_t i; + +if (!path || path[0] == '\0') +return ret; + +if (VIR_STRDUP(console->source.data.file.path, path) < 0) +return ret; + +if (console->targetType != VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) +return 0; + +for (i = 0; i < def->nserials; i++) { +virDomainChrDefPtr serial = def->serials[i]; + +if (serial->target.port == console->target.port && +VIR_STRDUP(serial->source.data.file.path, path) >= 0) { +ret = 0; +break; +} +} + +return ret; +} + static void libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback) { @@ -977,10 +1006,8 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback) &console); if (!ret) { VIR_FREE(chr->source.data.file.path); -if (console && console[0] != '\0') { -ignore_value(VIR_STRDUP(chr->source.data.file.path, -console)); -} +if (libxlConsoleSetSourcePath(vm->def, chr, console) < 0) +VIR_WARN("Failed to set console source path"); } VIR_FREE(console); } -- 2.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list