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 && + 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; + } 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; + } } } -- 1.7.3.4 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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;

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

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

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?

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?