[libvirt] [PATCH] libxl: Set path to console on domain startup
Hi, I'm trying to fix an issue when using OpenStack with libvirt+xen (libxenlight). OpenStack cannot access the console output of a Xen PV guest, because the XML generated by libvirt for a domain is missing the path to the pty. The path actually appear in the XML once one call virDomainOpenConsole(). The patch intend to get the path to the pty without having to call virDomainOpenConsole, so I've done the work in libxlDomainStart(). So I have a few question: Is libxlDomainStart will be called on restore/migrate/reboot? I guest the function libxlDomainOpenConsole() would not need to do the same work if the console path is settup properly. There is a bug report about this: https://bugzilla.redhat.com/show_bug.cgi?id=1170743 Regards, Anthony PERARD (1): libxl: Set path to console on domain startup. src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) -- Anthony PERARD
The path to the pty of a Xen PV console is set only in virDomainOpenConsole. But this is done too late. A call to virDomainGetXMLDesc done before OpenConsole will not have the path to the pty, but a call after OpenConsole will. e.g. of the current issue. Starting a domain with '<console type="pty"/>' Then: virDomainGetXMLDesc(): <devices> <console type='pty'> <target type='xen' port='0'/> </console> </devices> virDomainOpenConsole() virDomainGetXMLDesc(): <devices> <console type='pty' tty='/dev/pts/30'> <source path='/dev/pts/30'/> <target type='xen' port='0'/> </console> </devices> The patch intend to get the tty path on the first call of GetXMLDesc. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 9c62291..de56054 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto cleanup_dom; + if (vm->def->nconsoles) { + virDomainChrDefPtr chr = NULL; + chr = vm->def->consoles[0]; + if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + libxl_console_type console_type; + char *console = NULL; + console_type = + (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); + ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port, + console_type, &console); + if (!ret) + ignore_value(VIR_STRDUP(chr->source.data.file.path, console)); + VIR_FREE(console); + } + } + if (!start_paused) { libxl_domain_unpause(priv->ctx, domid); virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED); -- Anthony PERARD
Not really familiar with libvirt, but... On Fri, 5 Dec 2014 16:30:06 +0000 Anthony PERARD <anthony.perard@citrix.com> wrote:
The path to the pty of a Xen PV console is set only in virDomainOpenConsole. But this is done too late. A call to virDomainGetXMLDesc done before OpenConsole will not have the path to the pty, but a call after OpenConsole will.
e.g. of the current issue. Starting a domain with '<console type="pty"/>' Then: virDomainGetXMLDesc(): <devices> <console type='pty'> <target type='xen' port='0'/> </console> </devices> virDomainOpenConsole() virDomainGetXMLDesc(): <devices> <console type='pty' tty='/dev/pts/30'> <source path='/dev/pts/30'/> <target type='xen' port='0'/> </console> </devices>
The patch intend to get the tty path on the first call of GetXMLDesc.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 9c62291..de56054 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto cleanup_dom;
+ if (vm->def->nconsoles) { + virDomainChrDefPtr chr = NULL;
Pointless initializer. Possibly combine with following statement. -d
+ chr = vm->def->consoles[0]; + if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + libxl_console_type console_type; + char *console = NULL; + console_type = + (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); + ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port, + console_type, &console); + if (!ret) + ignore_value(VIR_STRDUP(chr->source.data.file.path, console)); + VIR_FREE(console); + } + } + if (!start_paused) { libxl_domain_unpause(priv->ctx, domid); virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED); -- Anthony PERARD
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Fri, 2014-12-05 at 16:30 +0000, Anthony PERARD wrote: Jim Fehlig maintains the libxl driver in libvirt, so you should CC him (I've done so here...)
The path to the pty of a Xen PV console is set only in virDomainOpenConsole. But this is done too late. A call to virDomainGetXMLDesc done before OpenConsole will not have the path to the pty, but a call after OpenConsole will.
e.g. of the current issue. Starting a domain with '<console type="pty"/>' Then: virDomainGetXMLDesc(): <devices> <console type='pty'> <target type='xen' port='0'/> </console> </devices> virDomainOpenConsole() virDomainGetXMLDesc(): <devices> <console type='pty' tty='/dev/pts/30'> <source path='/dev/pts/30'/> <target type='xen' port='0'/> </console> </devices>
The patch intend to get the tty path on the first call of GetXMLDesc.
Doesn't it actually do it on domain start (which makes more sense to me anyway).
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 9c62291..de56054 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto cleanup_dom;
+ if (vm->def->nconsoles) { + virDomainChrDefPtr chr = NULL; + chr = vm->def->consoles[0]; + if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + libxl_console_type console_type; + char *console = NULL; + console_type = + (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); + ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port, + console_type, &console); + if (!ret) + ignore_value(VIR_STRDUP(chr->source.data.file.path, console));
libxlDomainOpenConsole will strdup another (well, probably the same) value here, causing a leak I think, so you'll need some check there too I expect.
+ VIR_FREE(console); + } + } + if (!start_paused) { libxl_domain_unpause(priv->ctx, domid); virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED);
On Mon, Dec 08, 2014 at 11:59:44AM +0000, Ian Campbell wrote:
On Fri, 2014-12-05 at 16:30 +0000, Anthony PERARD wrote:
Jim Fehlig maintains the libxl driver in libvirt, so you should CC him (I've done so here...)
Thanks.
The path to the pty of a Xen PV console is set only in virDomainOpenConsole. But this is done too late. A call to virDomainGetXMLDesc done before OpenConsole will not have the path to the pty, but a call after OpenConsole will.
e.g. of the current issue. Starting a domain with '<console type="pty"/>' Then: virDomainGetXMLDesc(): <devices> <console type='pty'> <target type='xen' port='0'/> </console> </devices> virDomainOpenConsole() virDomainGetXMLDesc(): <devices> <console type='pty' tty='/dev/pts/30'> <source path='/dev/pts/30'/> <target type='xen' port='0'/> </console> </devices>
The patch intend to get the tty path on the first call of GetXMLDesc.
Doesn't it actually do it on domain start (which makes more sense to me anyway).
Just a wording issue. I meant: Have GetXMLDesc always return the path to the tty.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 9c62291..de56054 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto cleanup_dom;
+ if (vm->def->nconsoles) { + virDomainChrDefPtr chr = NULL; + chr = vm->def->consoles[0]; + if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + libxl_console_type console_type; + char *console = NULL; + console_type = + (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); + ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port, + console_type, &console); + if (!ret) + ignore_value(VIR_STRDUP(chr->source.data.file.path, console));
libxlDomainOpenConsole will strdup another (well, probably the same) value here, causing a leak I think, so you'll need some check there too I expect.
So, if OpenConsole is call twice, there will also be a leak? Or maybe it can not be called twice. Anyway, I though from the use of VIR_STRDUP there that it was safe to call VIR_STRDUP several times and it well free the destination. I might be wrong.
+ VIR_FREE(console); + } + } + if (!start_paused) { libxl_domain_unpause(priv->ctx, domid); virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED);
-- Anthony PERARD
On Mon, 2014-12-08 at 15:11 +0000, Anthony PERARD wrote:
The patch intend to get the tty path on the first call of GetXMLDesc.
Doesn't it actually do it on domain start (which makes more sense to me anyway).
Just a wording issue. I meant: Have GetXMLDesc always return the path to the tty.
Ah, yes, I get what you meant now.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 9c62291..de56054 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto cleanup_dom;
+ if (vm->def->nconsoles) { + virDomainChrDefPtr chr = NULL; + chr = vm->def->consoles[0]; + if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + libxl_console_type console_type; + char *console = NULL; + console_type = + (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); + ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port, + console_type, &console); + if (!ret) + ignore_value(VIR_STRDUP(chr->source.data.file.path, console));
libxlDomainOpenConsole will strdup another (well, probably the same) value here, causing a leak I think, so you'll need some check there too I expect.
So, if OpenConsole is call twice, there will also be a leak? Or maybe it can not be called twice.
I'm not sure, there may be an existing issue here I suppose.
Anyway, I though from the use of VIR_STRDUP there that it was safe to call VIR_STRDUP several times and it well free the destination. I might be wrong.
I wondered about that, but AFAICS VIR_STRDUP is a wrapper around virStrdup and virStrdup doesn't free any existing destination. Ian.
Anthony PERARD wrote:
The path to the pty of a Xen PV console is set only in virDomainOpenConsole. But this is done too late. A call to virDomainGetXMLDesc done before OpenConsole will not have the path to the pty, but a call after OpenConsole will.
Hi Anthony, Thanks for the patch. Can you address the comments made by others, my comments below, and provide a V2?
e.g. of the current issue. Starting a domain with '<console type="pty"/>' Then: virDomainGetXMLDesc(): <devices> <console type='pty'> <target type='xen' port='0'/> </console> </devices> virDomainOpenConsole() virDomainGetXMLDesc(): <devices> <console type='pty' tty='/dev/pts/30'> <source path='/dev/pts/30'/> <target type='xen' port='0'/> </console> </devices>
The patch intend to get the tty path on the first call of GetXMLDesc.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 9c62291..de56054 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto cleanup_dom;
+ if (vm->def->nconsoles) { + virDomainChrDefPtr chr = NULL; + chr = vm->def->consoles[0]; + if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + libxl_console_type console_type; + char *console = NULL; + console_type = + (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); + ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port, + console_type, &console); + if (!ret) + ignore_value(VIR_STRDUP(chr->source.data.file.path, console));
VIR_STRDUP will not free an existing value. You should VIR_FREE first, which btw can handle a NULL argument. And since you're initializing source.data.file.path when starting the domain, I think we can drop the similar code in libxlDomainOpenConsole(). Regards, Jim
+ VIR_FREE(console); + } + } + if (!start_paused) { libxl_domain_unpause(priv->ctx, domid); virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED);
On Mon, Dec 08, 2014 at 12:03:36PM -0700, Jim Fehlig wrote:
Anthony PERARD wrote:
The path to the pty of a Xen PV console is set only in virDomainOpenConsole. But this is done too late. A call to virDomainGetXMLDesc done before OpenConsole will not have the path to the pty, but a call after OpenConsole will.
Hi Anthony,
Thanks for the patch. Can you address the comments made by others, my comments below, and provide a V2?
Will do.
e.g. of the current issue. Starting a domain with '<console type="pty"/>' Then: virDomainGetXMLDesc(): <devices> <console type='pty'> <target type='xen' port='0'/> </console> </devices> virDomainOpenConsole() virDomainGetXMLDesc(): <devices> <console type='pty' tty='/dev/pts/30'> <source path='/dev/pts/30'/> <target type='xen' port='0'/> </console> </devices>
The patch intend to get the tty path on the first call of GetXMLDesc.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 9c62291..de56054 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto cleanup_dom;
+ if (vm->def->nconsoles) { + virDomainChrDefPtr chr = NULL; + chr = vm->def->consoles[0]; + if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { + libxl_console_type console_type; + char *console = NULL; + console_type = + (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ? + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV); + ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port, + console_type, &console); + if (!ret) + ignore_value(VIR_STRDUP(chr->source.data.file.path, console));
VIR_STRDUP will not free an existing value. You should VIR_FREE first, which btw can handle a NULL argument. And since you're initializing source.data.file.path when starting the domain, I think we can drop the similar code in libxlDomainOpenConsole().
I will do that. Thanks, -- Anthony PERARD
On 12/05/2014 05:30 PM, Anthony PERARD wrote:
Hi,
I'm trying to fix an issue when using OpenStack with libvirt+xen (libxenlight). OpenStack cannot access the console output of a Xen PV guest, because the XML generated by libvirt for a domain is missing the path to the pty. The path actually appear in the XML once one call virDomainOpenConsole().
The patch intend to get the path to the pty without having to call virDomainOpenConsole, so I've done the work in libxlDomainStart(). So I have a few question: Is libxlDomainStart will be called on restore/migrate/reboot? I guest the function libxlDomainOpenConsole() would not need to do the same work if the console path is settup properly.
There is a bug report about this: https://bugzilla.redhat.com/show_bug.cgi?id=1170743
Hi, you can leave the bugzilla link in the commit message, if somebody ever needs more context. (And the patch looks good to me, but I'm no libxl expert) Jan
Regards,
Anthony PERARD (1): libxl: Set path to console on domain startup.
src/libxl/libxl_domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
On Mon, Dec 08, 2014 at 04:40:04PM +0100, Ján Tomko wrote:
On 12/05/2014 05:30 PM, Anthony PERARD wrote:
Hi,
I'm trying to fix an issue when using OpenStack with libvirt+xen (libxenlight). OpenStack cannot access the console output of a Xen PV guest, because the XML generated by libvirt for a domain is missing the path to the pty. The path actually appear in the XML once one call virDomainOpenConsole().
The patch intend to get the path to the pty without having to call virDomainOpenConsole, so I've done the work in libxlDomainStart(). So I have a few question: Is libxlDomainStart will be called on restore/migrate/reboot? I guest the function libxlDomainOpenConsole() would not need to do the same work if the console path is settup properly.
There is a bug report about this: https://bugzilla.redhat.com/show_bug.cgi?id=1170743
Hi,
you can leave the bugzilla link in the commit message, if somebody ever needs more context.
Ok.
(And the patch looks good to me, but I'm no libxl expert)
Thanks, -- Anthony PERARD
Anthony PERARD wrote:
Hi,
I'm trying to fix an issue when using OpenStack with libvirt+xen (libxenlight). OpenStack cannot access the console output of a Xen PV guest, because the XML generated by libvirt for a domain is missing the path to the pty. The path actually appear in the XML once one call virDomainOpenConsole().
The patch intend to get the path to the pty without having to call virDomainOpenConsole, so I've done the work in libxlDomainStart(). So I have a few question: Is libxlDomainStart will be called on restore/migrate/reboot?
Yes.
I guest the function libxlDomainOpenConsole() would not need to do the same work if the console path is settup properly.
Yes, agreed. Regards, Jim
participants (5)
-
Anthony PERARD -
Don Koch -
Ian Campbell -
Jim Fehlig -
Ján Tomko