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

[Jim Fehlig]

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)
>  

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

[Cole Robinson]

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

[Jim Fehlig]

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 

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

[Cole Robinson]

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;
>  

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

[Jim Fehlig]

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

[Joao Martins]

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)
>>  );
>>  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

[Jim Fehlig]

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)
>  );
>  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

[Joao Martins]

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)
 );
 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