[libvirt] [PATCH] qemu: Adapt to new log format
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from: $ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7 to: $ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7 However, with current code we are not prepared for such change, which results in us being unable to start any domain. --- src/qemu/qemu_process.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eac6553..29bd082 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1431,22 +1431,43 @@ cleanup: * * char device redirected to /dev/pts/3 * + * However, since 1.4 the line we are looking for has changed to: + * + * char device <alias> redirected to /some/path + * * Returns -1 for error, 0 success, 1 continue reading */ static int qemuProcessExtractTTYPath(const char *haystack, size_t *offset, + const char *alias, char **path) { - static const char needle[] = "char device redirected to"; - char *tmp, *dev; + static const char *needle[] = {"char device", "redirected to"}; + const char *tmp, *dev; VIR_FREE(*path); /* First look for our magic string */ - if (!(tmp = strstr(haystack + *offset, needle))) { + if (!(tmp = strstr(haystack + *offset, needle[0]))) return 1; + + tmp += strlen(needle[0]); + virSkipSpaces(&tmp); + + if (STRPREFIX(tmp, "char")) { + /* we are dealing with new style */ + tmp += strlen("char"); + if (!STRPREFIX(tmp, alias)) + return 1; + + tmp += strlen(alias); + virSkipSpaces(&tmp); } - tmp += sizeof(needle); + + if (!STRPREFIX(tmp, needle[1])) + return 1; + + tmp += strlen(needle[1]); dev = tmp; /* @@ -1569,6 +1590,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, virDomainChrDefPtr chr = vm->def->serials[i]; if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { if ((ret = qemuProcessExtractTTYPath(output, &offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; } @@ -1579,6 +1601,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, virDomainChrDefPtr chr = vm->def->parallels[i]; if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { if ((ret = qemuProcessExtractTTYPath(output, &offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; } @@ -1589,6 +1612,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, virDomainChrDefPtr chr = vm->def->channels[i]; if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { if ((ret = qemuProcessExtractTTYPath(output, &offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; } @@ -1608,6 +1632,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY && chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) { if ((ret = qemuProcessExtractTTYPath(output, &offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; } -- 1.8.0.2
On 2012年12月29日 17:09, Michal Privoznik wrote:
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7
to:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7
However, with current code we are not prepared for such change, which results in us being unable to start any domain. --- src/qemu/qemu_process.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eac6553..29bd082 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1431,22 +1431,43 @@ cleanup: * * char device redirected to /dev/pts/3 * + * However, since 1.4 the line we are looking for has changed to: + * + * char device<alias> redirected to /some/path + * * Returns -1 for error, 0 success, 1 continue reading */ static int qemuProcessExtractTTYPath(const char *haystack, size_t *offset, + const char *alias, char **path) { - static const char needle[] = "char device redirected to"; - char *tmp, *dev; + static const char *needle[] = {"char device", "redirected to"}; + const char *tmp, *dev;
VIR_FREE(*path); /* First look for our magic string */ - if (!(tmp = strstr(haystack + *offset, needle))) { + if (!(tmp = strstr(haystack + *offset, needle[0]))) return 1; + + tmp += strlen(needle[0]); + virSkipSpaces(&tmp); + + if (STRPREFIX(tmp, "char")) {
I don't see why it's the new style with "char" here with regard to the new output string like "char device serial1 redirected to /dev/pts/7". Should it be below instead? if (!STRPREFIX(tmp, "redirected")) or if (STRPREFIX(tmp, alias))
+ /* we are dealing with new style */ + tmp += strlen("char"); + if (!STRPREFIX(tmp, alias)) + return 1; + + tmp += strlen(alias); + virSkipSpaces(&tmp); } - tmp += sizeof(needle); + + if (!STRPREFIX(tmp, needle[1])) + return 1; + + tmp += strlen(needle[1]); dev = tmp;
/* @@ -1569,6 +1590,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, virDomainChrDefPtr chr = vm->def->serials[i]; if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { if ((ret = qemuProcessExtractTTYPath(output,&offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; } @@ -1579,6 +1601,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, virDomainChrDefPtr chr = vm->def->parallels[i]; if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { if ((ret = qemuProcessExtractTTYPath(output,&offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; } @@ -1589,6 +1612,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, virDomainChrDefPtr chr = vm->def->channels[i]; if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) { if ((ret = qemuProcessExtractTTYPath(output,&offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; } @@ -1608,6 +1632,7 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm, if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY&& chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) { if ((ret = qemuProcessExtractTTYPath(output,&offset, + chr->info.alias, &chr->source.data.file.path)) != 0) return ret; }
On 30.12.2012 10:25, Osier Yang wrote:
On 2012年12月29日 17:09, Michal Privoznik wrote:
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7
to:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7
However, with current code we are not prepared for such change, which results in us being unable to start any domain. --- src/qemu/qemu_process.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eac6553..29bd082 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1431,22 +1431,43 @@ cleanup: * * char device redirected to /dev/pts/3 * + * However, since 1.4 the line we are looking for has changed to: + * + * char device<alias> redirected to /some/path + * * Returns -1 for error, 0 success, 1 continue reading */ static int qemuProcessExtractTTYPath(const char *haystack, size_t *offset, + const char *alias, char **path) { - static const char needle[] = "char device redirected to"; - char *tmp, *dev; + static const char *needle[] = {"char device", "redirected to"}; + const char *tmp, *dev;
VIR_FREE(*path); /* First look for our magic string */ - if (!(tmp = strstr(haystack + *offset, needle))) { + if (!(tmp = strstr(haystack + *offset, needle[0]))) return 1; + + tmp += strlen(needle[0]); + virSkipSpaces(&tmp); + + if (STRPREFIX(tmp, "char")) {
I don't see why it's the new style with "char" here with regard to the new output string like "char device serial1 redirected to /dev/pts/7". Should it be below instead?
if (!STRPREFIX(tmp, "redirected")) or if (STRPREFIX(tmp, alias))
I should have documented that. If you take look at qemuBuildChrChardevStr() you can see, that all char devices IDs are constructed as "id=char%s", where %s is substituted device alias. Hence qemu sees 'charalias0' instead of bare 'alias0'. Therefore we should check for 'char' prefix. Michal
On 2012年12月30日 18:26, Michal Privoznik wrote:
On 30.12.2012 10:25, Osier Yang wrote:
On 2012年12月29日 17:09, Michal Privoznik wrote:
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7
to:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7
However, with current code we are not prepared for such change, which results in us being unable to start any domain. --- src/qemu/qemu_process.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eac6553..29bd082 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1431,22 +1431,43 @@ cleanup: * * char device redirected to /dev/pts/3 * + * However, since 1.4 the line we are looking for has changed to: + * + * char device<alias> redirected to /some/path + * * Returns -1 for error, 0 success, 1 continue reading */ static int qemuProcessExtractTTYPath(const char *haystack, size_t *offset, + const char *alias, char **path) { - static const char needle[] = "char device redirected to"; - char *tmp, *dev; + static const char *needle[] = {"char device", "redirected to"}; + const char *tmp, *dev;
VIR_FREE(*path); /* First look for our magic string */ - if (!(tmp = strstr(haystack + *offset, needle))) { + if (!(tmp = strstr(haystack + *offset, needle[0]))) return 1; + + tmp += strlen(needle[0]); + virSkipSpaces(&tmp); + + if (STRPREFIX(tmp, "char")) {
I don't see why it's the new style with "char" here with regard to the new output string like "char device serial1 redirected to /dev/pts/7". Should it be below instead?
if (!STRPREFIX(tmp, "redirected")) or if (STRPREFIX(tmp, alias))
I should have documented that. If you take look at qemuBuildChrChardevStr() you can see, that all char devices IDs are constructed as "id=char%s", where %s is substituted device alias. Hence qemu sees 'charalias0' instead of bare 'alias0'. Therefore we should check for 'char' prefix.
Oh, I missed that, agreed with you that better to add a comment for that, ACK with the comment added. Osier
On 30.12.2012 11:56, Osier Yang wrote:
On 2012年12月30日 18:26, Michal Privoznik wrote:
On 30.12.2012 10:25, Osier Yang wrote:
On 2012年12月29日 17:09, Michal Privoznik wrote:
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7
to:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7
However, with current code we are not prepared for such change, which results in us being unable to start any domain. --- src/qemu/qemu_process.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eac6553..29bd082 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1431,22 +1431,43 @@ cleanup: * * char device redirected to /dev/pts/3 * + * However, since 1.4 the line we are looking for has changed to: + * + * char device<alias> redirected to /some/path + * * Returns -1 for error, 0 success, 1 continue reading */ static int qemuProcessExtractTTYPath(const char *haystack, size_t *offset, + const char *alias, char **path) { - static const char needle[] = "char device redirected to"; - char *tmp, *dev; + static const char *needle[] = {"char device", "redirected to"}; + const char *tmp, *dev;
VIR_FREE(*path); /* First look for our magic string */ - if (!(tmp = strstr(haystack + *offset, needle))) { + if (!(tmp = strstr(haystack + *offset, needle[0]))) return 1; + + tmp += strlen(needle[0]); + virSkipSpaces(&tmp); + + if (STRPREFIX(tmp, "char")) {
I don't see why it's the new style with "char" here with regard to the new output string like "char device serial1 redirected to /dev/pts/7". Should it be below instead?
if (!STRPREFIX(tmp, "redirected")) or if (STRPREFIX(tmp, alias))
I should have documented that. If you take look at qemuBuildChrChardevStr() you can see, that all char devices IDs are constructed as "id=char%s", where %s is substituted device alias. Hence qemu sees 'charalias0' instead of bare 'alias0'. Therefore we should check for 'char' prefix.
Oh, I missed that, agreed with you that better to add a comment for that, ACK with the comment added.
Osier
Added and pushed. Thanks. Michal
On 12/29/2012 02:09 AM, Michal Privoznik wrote:
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7
to:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7
Why are we even scraping stdout? If we are using qemu 1.2 or newer, isn't this information also available from a QMP monitor command? If so, we should modify the code to use QMP (which will work regardless of the change in output in qemu 1.4), and leave the old code for scraping stdout of qemu 1.1 untouched. I'm a bit late in reviewing this, since you already pushed, but I'm inclined to NACK it if we can come up with a better solution using QMP. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Sat, Dec 29, 2012 at 10:09:41AM +0100, Michal Privoznik wrote:
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7
to:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7
However, with current code we are not prepared for such change, which results in us being unable to start any domain.
Huh, we should not be relying on this data at all with current QEMU. We switched to using 'info chardev' / 'query-chardev' years ago. NACK to this patch. You need to find out what it is not using the monitor for this. 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 :|
On 02.01.2013 11:25, Daniel P. Berrange wrote:
On Sat, Dec 29, 2012 at 10:09:41AM +0100, Michal Privoznik wrote:
Since 586502189edf9fd0f89a83de96717a2ea826fdb0 qemu commit, the log lines reporting chardev's path has changed from:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device redirected to /dev/pts/5 char device redirected to /dev/pts/6 char device redirected to /dev/pts/7
to:
$ ./x86_64-softmmu/qemu-system-x86_64 -serial pty -serial pty -monitor pty char device compat_monitor0 redirected to /dev/pts/5 char device serial0 redirected to /dev/pts/6 char device serial1 redirected to /dev/pts/7
However, with current code we are not prepared for such change, which results in us being unable to start any domain.
Huh, we should not be relying on this data at all with current QEMU. We switched to using 'info chardev' / 'query-chardev' years ago.
NACK to this patch. You need to find out what it is not using the monitor for this.
Daniel
Okay, I've reverted the patch and will search for the root cause. Michal
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Osier Yang