Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-13 Thread Jan Kiszka 
On 2012-03-09 19:11, Jan Kiszka wrote:
 On 2012-03-09 19:02, Daniel P. Berrange wrote:
 On Fri, Mar 09, 2012 at 06:58:06PM +0100, Jan Kiszka wrote:
 On 2012-03-09 18:53, Daniel P. Berrange wrote:
 On Fri, Mar 09, 2012 at 06:48:42PM +0100, Jan Kiszka wrote:
 On 2011-11-16 14:14, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.

 ...

 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 2882ef8..e0b1824 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  
  for (i = 0 ; i  vm-def-nconsoles ; i++) {
  virDomainChrDefPtr chr = vm-def-consoles[i];
 -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 -chr-targetType == 
 VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 -if ((ret = qemuProcessExtractTTYPath(output, offset,
 - 
 chr-source.data.file.path)) != 0)
 +/* For historical reasons, console[0] can be just an alias
 + * for serial[0]; That's why we need to update it as well */
 +if (i == 0  vm-def-nserials 
 +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 +chr-targetType == 
 VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
 +if ((ret = virDomainChrSourceDefCopy(chr-source,
 + 
 ((vm-def-serials[0])-source))) != 0)
  return ret;
 +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;

 This unconditional setting of TYPE_PTY breaks serial on stdio (we use
 this to easily fold guest into host logs). Can you explain why the
 copied source.type of serial[0] is not always correct? Or are we already
 in the wrong branch for a

 serial type='stdio'/serial

 configuration?

 Yeah I think this is a bug. The first serial element should match the
 first console exactly, with targetType==serial. We shouldn't be
 forcing it to type=pty

 So, if vm-def-serials[0])-source.type != VIR_DOMAIN_CHR_TYPE_PTY, we
 should skip this fixup branch?

 No, we should do the fix for all types. serial[0] and console[0] should
 be identical, unless the console[0] targetType != TARGET_TYPE_SERIAL
 
 Well, libvirt is alien to me, and at this point I'm lost in fixups. :)
 Do you mean simply dropping the source.type = TYPE_PTY line?
 

Ping?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-13 Thread Daniel P. Berrange 
On Fri, Mar 09, 2012 at 07:11:05PM +0100, Jan Kiszka wrote:
 On 2012-03-09 19:02, Daniel P. Berrange wrote:
  On Fri, Mar 09, 2012 at 06:58:06PM +0100, Jan Kiszka wrote:
  On 2012-03-09 18:53, Daniel P. Berrange wrote:
  On Fri, Mar 09, 2012 at 06:48:42PM +0100, Jan Kiszka wrote:
  On 2011-11-16 14:14, Michal Privoznik wrote:
  Now, when we support multiple consoles per domain,
  the vm-def-console[0] can still remain an alias
  for vm-def-serial[0]; However, we need to copy
  it's source definition as well otherwise we'll regress
  on virDomainOpenConsole.
 
  ...
 
  diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
  index 2882ef8..e0b1824 100644
  --- a/src/qemu/qemu_process.c
  +++ b/src/qemu/qemu_process.c
  @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr 
  vm,
   
   for (i = 0 ; i  vm-def-nconsoles ; i++) {
   virDomainChrDefPtr chr = vm-def-consoles[i];
  -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
  -chr-targetType == 
  VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
  -if ((ret = qemuProcessExtractTTYPath(output, offset,
  - 
  chr-source.data.file.path)) != 0)
  +/* For historical reasons, console[0] can be just an alias
  + * for serial[0]; That's why we need to update it as well */
  +if (i == 0  vm-def-nserials 
  +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
  +chr-targetType == 
  VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
  +if ((ret = virDomainChrSourceDefCopy(chr-source,
  + 
  ((vm-def-serials[0])-source))) != 0)
   return ret;
  +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;
 
  This unconditional setting of TYPE_PTY breaks serial on stdio (we use
  this to easily fold guest into host logs). Can you explain why the
  copied source.type of serial[0] is not always correct? Or are we already
  in the wrong branch for a
 
  serial type='stdio'/serial
 
  configuration?
 
  Yeah I think this is a bug. The first serial element should match the
  first console exactly, with targetType==serial. We shouldn't be
  forcing it to type=pty
 
  So, if vm-def-serials[0])-source.type != VIR_DOMAIN_CHR_TYPE_PTY, we
  should skip this fixup branch?
  
  No, we should do the fix for all types. serial[0] and console[0] should
  be identical, unless the console[0] targetType != TARGET_TYPE_SERIAL
 
 Well, libvirt is alien to me, and at this point I'm lost in fixups. :)
 Do you mean simply dropping the source.type = TYPE_PTY line?

Yes, I think that line can just be removed.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-09 Thread Jan Kiszka 
On 2011-11-16 14:14, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.

...

 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 2882ef8..e0b1824 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  
  for (i = 0 ; i  vm-def-nconsoles ; i++) {
  virDomainChrDefPtr chr = vm-def-consoles[i];
 -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 -chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 -if ((ret = qemuProcessExtractTTYPath(output, offset,
 - 
 chr-source.data.file.path)) != 0)
 +/* For historical reasons, console[0] can be just an alias
 + * for serial[0]; That's why we need to update it as well */
 +if (i == 0  vm-def-nserials 
 +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 +chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
 +if ((ret = virDomainChrSourceDefCopy(chr-source,
 + 
 ((vm-def-serials[0])-source))) != 0)
  return ret;
 +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;

This unconditional setting of TYPE_PTY breaks serial on stdio (we use
this to easily fold guest into host logs). Can you explain why the
copied source.type of serial[0] is not always correct? Or are we already
in the wrong branch for a

serial type='stdio'/serial

configuration?

Jan

 +} else {
 +if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 +chr-targetType == 
 VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 +if ((ret = qemuProcessExtractTTYPath(output, offset,
 + 
 chr-source.data.file.path)) != 0)
 +return ret;
 +}
  }
  }
  

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-09 Thread Daniel P. Berrange 
On Fri, Mar 09, 2012 at 06:48:42PM +0100, Jan Kiszka wrote:
 On 2011-11-16 14:14, Michal Privoznik wrote:
  Now, when we support multiple consoles per domain,
  the vm-def-console[0] can still remain an alias
  for vm-def-serial[0]; However, we need to copy
  it's source definition as well otherwise we'll regress
  on virDomainOpenConsole.
 
 ...
 
  diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
  index 2882ef8..e0b1824 100644
  --- a/src/qemu/qemu_process.c
  +++ b/src/qemu/qemu_process.c
  @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
   
   for (i = 0 ; i  vm-def-nconsoles ; i++) {
   virDomainChrDefPtr chr = vm-def-consoles[i];
  -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
  -chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
  -if ((ret = qemuProcessExtractTTYPath(output, offset,
  - 
  chr-source.data.file.path)) != 0)
  +/* For historical reasons, console[0] can be just an alias
  + * for serial[0]; That's why we need to update it as well */
  +if (i == 0  vm-def-nserials 
  +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
  +chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
  +if ((ret = virDomainChrSourceDefCopy(chr-source,
  + 
  ((vm-def-serials[0])-source))) != 0)
   return ret;
  +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;
 
 This unconditional setting of TYPE_PTY breaks serial on stdio (we use
 this to easily fold guest into host logs). Can you explain why the
 copied source.type of serial[0] is not always correct? Or are we already
 in the wrong branch for a
 
 serial type='stdio'/serial
 
 configuration?

Yeah I think this is a bug. The first serial element should match the
first console exactly, with targetType==serial. We shouldn't be
forcing it to type=pty


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-09 Thread Jan Kiszka 
On 2012-03-09 18:53, Daniel P. Berrange wrote:
 On Fri, Mar 09, 2012 at 06:48:42PM +0100, Jan Kiszka wrote:
 On 2011-11-16 14:14, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.

 ...

 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 2882ef8..e0b1824 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  
  for (i = 0 ; i  vm-def-nconsoles ; i++) {
  virDomainChrDefPtr chr = vm-def-consoles[i];
 -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 -chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 -if ((ret = qemuProcessExtractTTYPath(output, offset,
 - 
 chr-source.data.file.path)) != 0)
 +/* For historical reasons, console[0] can be just an alias
 + * for serial[0]; That's why we need to update it as well */
 +if (i == 0  vm-def-nserials 
 +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 +chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
 +if ((ret = virDomainChrSourceDefCopy(chr-source,
 + 
 ((vm-def-serials[0])-source))) != 0)
  return ret;
 +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;

 This unconditional setting of TYPE_PTY breaks serial on stdio (we use
 this to easily fold guest into host logs). Can you explain why the
 copied source.type of serial[0] is not always correct? Or are we already
 in the wrong branch for a

 serial type='stdio'/serial

 configuration?
 
 Yeah I think this is a bug. The first serial element should match the
 first console exactly, with targetType==serial. We shouldn't be
 forcing it to type=pty

So, if vm-def-serials[0])-source.type != VIR_DOMAIN_CHR_TYPE_PTY, we
should skip this fixup branch?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-09 Thread Jan Kiszka 
On 2012-03-09 18:58, Jan Kiszka wrote:
 On 2012-03-09 18:53, Daniel P. Berrange wrote:
 On Fri, Mar 09, 2012 at 06:48:42PM +0100, Jan Kiszka wrote:
 On 2011-11-16 14:14, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.

 ...

 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 2882ef8..e0b1824 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  
  for (i = 0 ; i  vm-def-nconsoles ; i++) {
  virDomainChrDefPtr chr = vm-def-consoles[i];
 -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 -chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) 
 {
 -if ((ret = qemuProcessExtractTTYPath(output, offset,
 - 
 chr-source.data.file.path)) != 0)
 +/* For historical reasons, console[0] can be just an alias
 + * for serial[0]; That's why we need to update it as well */
 +if (i == 0  vm-def-nserials 
 +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 +chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) 
 {
 +if ((ret = virDomainChrSourceDefCopy(chr-source,
 + 
 ((vm-def-serials[0])-source))) != 0)
  return ret;
 +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;

 This unconditional setting of TYPE_PTY breaks serial on stdio (we use
 this to easily fold guest into host logs). Can you explain why the
 copied source.type of serial[0] is not always correct? Or are we already
 in the wrong branch for a

 serial type='stdio'/serial

 configuration?

 Yeah I think this is a bug. The first serial element should match the
 first console exactly, with targetType==serial. We shouldn't be
 forcing it to type=pty
 
 So, if vm-def-serials[0])-source.type != VIR_DOMAIN_CHR_TYPE_PTY, we
 should skip this fixup branch?
 

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1ac892f..7aafb50 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1372,11 +1372,11 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  * for serial[0]; That's why we need to update it as well */
 if (i == 0  vm-def-nserials 
 chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
-chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
+chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL 
+vm-def-serials[0]-source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
 if ((ret = virDomainChrSourceDefCopy(chr-source,
  
((vm-def-serials[0])-source))) != 0)
 return ret;
-chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;
 } else {
 if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {

?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-09 Thread Daniel P. Berrange 
On Fri, Mar 09, 2012 at 06:58:06PM +0100, Jan Kiszka wrote:
 On 2012-03-09 18:53, Daniel P. Berrange wrote:
  On Fri, Mar 09, 2012 at 06:48:42PM +0100, Jan Kiszka wrote:
  On 2011-11-16 14:14, Michal Privoznik wrote:
  Now, when we support multiple consoles per domain,
  the vm-def-console[0] can still remain an alias
  for vm-def-serial[0]; However, we need to copy
  it's source definition as well otherwise we'll regress
  on virDomainOpenConsole.
 
  ...
 
  diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
  index 2882ef8..e0b1824 100644
  --- a/src/qemu/qemu_process.c
  +++ b/src/qemu/qemu_process.c
  @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
   
   for (i = 0 ; i  vm-def-nconsoles ; i++) {
   virDomainChrDefPtr chr = vm-def-consoles[i];
  -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
  -chr-targetType == 
  VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
  -if ((ret = qemuProcessExtractTTYPath(output, offset,
  - 
  chr-source.data.file.path)) != 0)
  +/* For historical reasons, console[0] can be just an alias
  + * for serial[0]; That's why we need to update it as well */
  +if (i == 0  vm-def-nserials 
  +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
  +chr-targetType == 
  VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
  +if ((ret = virDomainChrSourceDefCopy(chr-source,
  + 
  ((vm-def-serials[0])-source))) != 0)
   return ret;
  +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;
 
  This unconditional setting of TYPE_PTY breaks serial on stdio (we use
  this to easily fold guest into host logs). Can you explain why the
  copied source.type of serial[0] is not always correct? Or are we already
  in the wrong branch for a
 
  serial type='stdio'/serial
 
  configuration?
  
  Yeah I think this is a bug. The first serial element should match the
  first console exactly, with targetType==serial. We shouldn't be
  forcing it to type=pty
 
 So, if vm-def-serials[0])-source.type != VIR_DOMAIN_CHR_TYPE_PTY, we
 should skip this fixup branch?

No, we should do the fix for all types. serial[0] and console[0] should
be identical, unless the console[0] targetType != TARGET_TYPE_SERIAL


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2012-03-09 Thread Jan Kiszka 
On 2012-03-09 19:02, Daniel P. Berrange wrote:
 On Fri, Mar 09, 2012 at 06:58:06PM +0100, Jan Kiszka wrote:
 On 2012-03-09 18:53, Daniel P. Berrange wrote:
 On Fri, Mar 09, 2012 at 06:48:42PM +0100, Jan Kiszka wrote:
 On 2011-11-16 14:14, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.

 ...

 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 2882ef8..e0b1824 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  
  for (i = 0 ; i  vm-def-nconsoles ; i++) {
  virDomainChrDefPtr chr = vm-def-consoles[i];
 -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 -chr-targetType == 
 VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 -if ((ret = qemuProcessExtractTTYPath(output, offset,
 - 
 chr-source.data.file.path)) != 0)
 +/* For historical reasons, console[0] can be just an alias
 + * for serial[0]; That's why we need to update it as well */
 +if (i == 0  vm-def-nserials 
 +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 +chr-targetType == 
 VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
 +if ((ret = virDomainChrSourceDefCopy(chr-source,
 + 
 ((vm-def-serials[0])-source))) != 0)
  return ret;
 +chr-source.type = VIR_DOMAIN_CHR_TYPE_PTY;

 This unconditional setting of TYPE_PTY breaks serial on stdio (we use
 this to easily fold guest into host logs). Can you explain why the
 copied source.type of serial[0] is not always correct? Or are we already
 in the wrong branch for a

 serial type='stdio'/serial

 configuration?

 Yeah I think this is a bug. The first serial element should match the
 first console exactly, with targetType==serial. We shouldn't be
 forcing it to type=pty

 So, if vm-def-serials[0])-source.type != VIR_DOMAIN_CHR_TYPE_PTY, we
 should skip this fixup branch?
 
 No, we should do the fix for all types. serial[0] and console[0] should
 be identical, unless the console[0] targetType != TARGET_TYPE_SERIAL

Well, libvirt is alien to me, and at this point I'm lost in fixups. :)
Do you mean simply dropping the source.type = TYPE_PTY line?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2011-11-21 Thread Michal Privoznik 
On 17.11.2011 00:33, Eric Blake wrote:
 On 11/16/2011 06:14 AM, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.
 ---
  src/conf/domain_conf.c   |   72 
 ++
  src/conf/domain_conf.h   |2 +
  src/libvirt_private.syms |1 +
  src/qemu/qemu_process.c  |   19 +--
  4 files changed, 90 insertions(+), 4 deletions(-)
 
 ACK if you squash this in to avoid the memory leak (you may want to wait
 for Dave to confirm that squashing this in still works in his testing):
 

It works for me with squashed part. The 'reproducer' is really simple -
just run 'virsh console $vm' where vm has console defined like this:

domain type='kvm'
  ...
  devices
...
serial type='pty'
  target port='0'/
/serial
console type='pty'
  target type='serial' port='0'/
/console
...
  /devices
/domain


I assume it works for Dave as well; so I am pushing it.

Thanks

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2011-11-21 Thread Dave Allan 
On Mon, Nov 21, 2011 at 02:52:21PM +0100, Michal Privoznik wrote:
 On 17.11.2011 00:33, Eric Blake wrote:
  On 11/16/2011 06:14 AM, Michal Privoznik wrote:
  Now, when we support multiple consoles per domain,
  the vm-def-console[0] can still remain an alias
  for vm-def-serial[0]; However, we need to copy
  it's source definition as well otherwise we'll regress
  on virDomainOpenConsole.
  ---
   src/conf/domain_conf.c   |   72 
  ++
   src/conf/domain_conf.h   |2 +
   src/libvirt_private.syms |1 +
   src/qemu/qemu_process.c  |   19 +--
   4 files changed, 90 insertions(+), 4 deletions(-)
  
  ACK if you squash this in to avoid the memory leak (you may want to wait
  for Dave to confirm that squashing this in still works in his testing):
  
 
 It works for me with squashed part. The 'reproducer' is really simple -
 just run 'virsh console $vm' where vm has console defined like this:
 
 domain type='kvm'
   ...
   devices
 ...
 serial type='pty'
   target port='0'/
 /serial
 console type='pty'
   target type='serial' port='0'/
 /console
 ...
   /devices
 /domain
 
 
 I assume it works for Dave as well; so I am pushing it.

Confirmed, it still works for me.

Dave

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2011-11-16 Thread Dave Allan 
I haven't code reviewed it, but it fixes serial console on my system,
so ack to the functionality.

Dave

On Wed, Nov 16, 2011 at 02:14:52PM +0100, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.
 ---
  src/conf/domain_conf.c   |   72 
 ++
  src/conf/domain_conf.h   |2 +
  src/libvirt_private.syms |1 +
  src/qemu/qemu_process.c  |   19 +--
  4 files changed, 90 insertions(+), 4 deletions(-)
 
 diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 index 6b78d97..9b2eb86 100644
 --- a/src/conf/domain_conf.c
 +++ b/src/conf/domain_conf.c
 @@ -956,6 +956,78 @@ virDomainChrSourceDefClear(virDomainChrSourceDefPtr def)
  }
  }
  
 +int
 +virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
 +  virDomainChrSourceDefPtr src)
 +{
 +if (!dest || !src)
 +return -1;
 +
 +switch (src-type) {
 +case VIR_DOMAIN_CHR_TYPE_PTY:
 +case VIR_DOMAIN_CHR_TYPE_DEV:
 +case VIR_DOMAIN_CHR_TYPE_FILE:
 +case VIR_DOMAIN_CHR_TYPE_PIPE:
 +if (src-data.file.path 
 +!(dest-data.file.path = strdup(src-data.file.path))) {
 +virReportOOMError();
 +return -1;
 +}
 +break;
 +
 +case VIR_DOMAIN_CHR_TYPE_UDP:
 +if (src-data.udp.bindHost 
 +!(dest-data.udp.bindHost = strdup(src-data.udp.bindHost))) {
 +virReportOOMError();
 +return -1;
 +}
 +
 +if (src-data.udp.bindService 
 +!(dest-data.udp.bindService = 
 strdup(src-data.udp.bindService))) {
 +virReportOOMError();
 +return -1;
 +}
 +
 +if (src-data.udp.connectHost 
 +!(dest-data.udp.connectHost = 
 strdup(src-data.udp.connectHost))) {
 +virReportOOMError();
 +return -1;
 +}
 +
 +
 +if (src-data.udp.connectService 
 +!(dest-data.udp.connectService = 
 strdup(src-data.udp.connectService))) {
 +virReportOOMError();
 +return -1;
 +}
 +break;
 +
 +case VIR_DOMAIN_CHR_TYPE_TCP:
 +if (src-data.tcp.host 
 +!(dest-data.tcp.host = strdup(src-data.tcp.host))) {
 +virReportOOMError();
 +return -1;
 +}
 +
 +if (src-data.tcp.service 
 +!(dest-data.tcp.service = strdup(src-data.tcp.service))) {
 +virReportOOMError();
 +return -1;
 +}
 +break;
 +
 +case VIR_DOMAIN_CHR_TYPE_UNIX:
 +if (src-data.nix.path 
 +!(dest-data.nix.path = strdup(src-data.nix.path))) {
 +virReportOOMError();
 +return -1;
 +}
 +break;
 +}
 +
 +return 0;
 +}
 +
  void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
  {
  if (!def)
 diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
 index c360674..76a8dd7 100644
 --- a/src/conf/domain_conf.h
 +++ b/src/conf/domain_conf.h
 @@ -1662,6 +1662,8 @@ void virDomainNetDefFree(virDomainNetDefPtr def);
  void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def);
  void virDomainChrDefFree(virDomainChrDefPtr def);
  void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def);
 +int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src,
 +  virDomainChrSourceDefPtr dest);
  void virDomainSoundDefFree(virDomainSoundDefPtr def);
  void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def);
  void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def);
 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 index b9d537e..e6ccf9d 100644
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -242,6 +242,7 @@ virDomainChrConsoleTargetTypeToString;
  virDomainChrDefForeach;
  virDomainChrDefFree;
  virDomainChrDefNew;
 +virDomainChrSourceDefCopy;
  virDomainChrSourceDefFree;
  virDomainChrSpicevmcTypeFromString;
  virDomainChrSpicevmcTypeToString;
 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 2882ef8..e0b1824 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  
  for (i = 0 ; i  vm-def-nconsoles ; i++) {
  virDomainChrDefPtr chr = vm-def-consoles[i];
 -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 -chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 -if ((ret = qemuProcessExtractTTYPath(output, offset,
 - 
 chr-source.data.file.path)) != 0)
 +/* For historical reasons, console[0] can be just an alias
 + * for serial[0]; That's why we need to update it as well */
 +if (i == 0  vm-def-nserials

Re: [libvirt] [PATCH] qemu: Copy console definition from serial

2011-11-16 Thread Eric Blake 
On 11/16/2011 06:14 AM, Michal Privoznik wrote:
 Now, when we support multiple consoles per domain,
 the vm-def-console[0] can still remain an alias
 for vm-def-serial[0]; However, we need to copy
 it's source definition as well otherwise we'll regress
 on virDomainOpenConsole.
 ---
  src/conf/domain_conf.c   |   72 
 ++
  src/conf/domain_conf.h   |2 +
  src/libvirt_private.syms |1 +
  src/qemu/qemu_process.c  |   19 +--
  4 files changed, 90 insertions(+), 4 deletions(-)

I do agree that we need the deep copy - when we first connect to qemu,
we do a lookup to see what ptys got allocated, and modify vm-def
accordingly.  If vm-def-serials[0] is modified by the pty lookup, and
vm-def-consoles[0] is an alias to serials[0], then the allocations
that were done to modify the serials lookup have to be cloned to
vm-def-consoles[0].

 
 diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 index 6b78d97..9b2eb86 100644
 --- a/src/conf/domain_conf.c
 +++ b/src/conf/domain_conf.c
 @@ -956,6 +956,78 @@ virDomainChrSourceDefClear(virDomainChrSourceDefPtr def)
  }
  }
  
 +int
 +virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
 +  virDomainChrSourceDefPtr src)
 +{
 +if (!dest || !src)
 +return -1;
 +
 +switch (src-type) {
 +case VIR_DOMAIN_CHR_TYPE_PTY:
 +case VIR_DOMAIN_CHR_TYPE_DEV:
 +case VIR_DOMAIN_CHR_TYPE_FILE:
 +case VIR_DOMAIN_CHR_TYPE_PIPE:
 +if (src-data.file.path 
 +!(dest-data.file.path = strdup(src-data.file.path))) {
 +virReportOOMError();
 +return -1;
 +}
 +break;
 +
 +case VIR_DOMAIN_CHR_TYPE_UDP:
 +if (src-data.udp.bindHost 
 +!(dest-data.udp.bindHost = strdup(src-data.udp.bindHost))) {
 +virReportOOMError();
 +return -1;
 +}

Hmm.  If this strdup() succeeds,

 +
 +if (src-data.udp.bindService 
 +!(dest-data.udp.bindService = 
 strdup(src-data.udp.bindService))) {
 +virReportOOMError();
 +return -1;
 +}

but this fails, then the burden is on the caller to free up dest on
error in order to avoid a memory leak.  But...

 +++ b/src/qemu/qemu_process.c
 @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
  
  for (i = 0 ; i  vm-def-nconsoles ; i++) {
  virDomainChrDefPtr chr = vm-def-consoles[i];
 -if (chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY 
 -chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 -if ((ret = qemuProcessExtractTTYPath(output, offset,
 - 
 chr-source.data.file.path)) != 0)
 +/* For historical reasons, console[0] can be just an alias
 + * for serial[0]; That's why we need to update it as well */
 +if (i == 0  vm-def-nserials 
 +chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
 +chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
 +if ((ret = virDomainChrSourceDefCopy(chr-source,
 + 
 ((vm-def-serials[0])-source))) != 0)
  return ret;

at first glance, the lone caller doesn't directly free dest.  On the
other hand, the lone caller passed in dest of vm-def-consoles[0],
which eventually gets cleaned up when vm-def is freed, so you've
escaped the original leak that I was worried might exist.  However, if
qemuProcessFindCharDevicePTYs ever gets called multiple times on the
same vm-def, then the second call overwrites the pointer from the first
call, and you have a memory leak in that aspect; and since it is
cross-file between allocation and error path, it's hard to chase down.

ACK if you squash this in to avoid the memory leak (you may want to wait
for Dave to confirm that squashing this in still works in his testing):

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 9b2eb86..e6f97b8 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -956,6 +956,8 @@ virDomainChrSourceDefClear(virDomainChrSourceDefPtr def)
 }
 }

+/* Deep copies the contents of src into dest.  Return -1 and report
+ * error on failure.  */
 int
 virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
   virDomainChrSourceDefPtr src)
@@ -963,6 +965,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
 if (!dest || !src)
 return -1;

+virDomainChrSourceDefClear(dest);
+
 switch (src-type) {
 case VIR_DOMAIN_CHR_TYPE_PTY:
 case VIR_DOMAIN_CHR_TYPE_DEV:


-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list