[libvirt] [PATCH 0/2]
Hi. I used the rawhide RPMs to take out the DLLs for a test drive on Windows 64bit the other day. While trying to read the "screenshot" from the test domain using the test:///default connection, I only received 5 Bytes of the libvirtLogo.png file. Looking at the test driver, the condition piqued my interest, but wasn't the cause of the short read, only a bit odd. See patch #1. The actual cause was that the file was opened in text mode and the seventh byte was a 0x1A which triggered EOF on the FD. See patch #2. Claudio Bley (2): test: fix call to virFDStreamOpenFile in testDomainScreenshot Always open files in binary mode in virFDStreamOpenFileInternal src/fdstream.c | 2 +- src/test/test_driver.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 1.8.3.1
N.B. This had no ill effects as long as O_RDONLY is defined to to be 0, such that the expression (O_RDONLY < 0) yielded 0 again. Signed-off-by: Claudio Bley <cbley@av-test.de> --- src/test/test_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 47c9d38..e1197c9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5862,7 +5862,7 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, if (VIR_STRDUP(ret, "image/png") < 0) return NULL; - if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY < 0)) + if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY) < 0) VIR_FREE(ret); return ret; -- 1.8.3.1
On 09/24/2013 03:57 AM, Claudio Bley wrote:
N.B. This had no ill effects as long as O_RDONLY is defined to to be 0, such that the expression (O_RDONLY < 0) yielded 0 again.
Signed-off-by: Claudio Bley <cbley@av-test.de> --- src/test/test_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 47c9d38..e1197c9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5862,7 +5862,7 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, if (VIR_STRDUP(ret, "image/png") < 0) return NULL;
- if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY < 0)) + if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY) < 0)
ACK. Embarrassing that even Coverity doesn't flag this blatant bug. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
At Tue, 24 Sep 2013 05:53:41 -0600, Eric Blake wrote:
On 09/24/2013 03:57 AM, Claudio Bley wrote:
N.B. This had no ill effects as long as O_RDONLY is defined to to be 0, such that the expression (O_RDONLY < 0) yielded 0 again.
Signed-off-by: Claudio Bley <cbley@av-test.de> --- src/test/test_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 47c9d38..e1197c9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5862,7 +5862,7 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, if (VIR_STRDUP(ret, "image/png") < 0) return NULL;
- if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY < 0)) + if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY) < 0)
ACK. Embarrassing that even Coverity doesn't flag this blatant bug.
On 09/24/2013 03:57 AM, Claudio Bley wrote:
On win32, using text mode for binary files might result in short reads since ASCII character 0x1A is interpreted as EOF. Also, it could lead to problems using the seek functions because of the \r handling.
Signed-off-by: Claudio Bley <cbley@av-test.de> --- src/fdstream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
Thanks, both pushed now. Claudio n-- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On win32, using text mode for binary files might result in short reads since ASCII character 0x1A is interpreted as EOF. Also, it could lead to problems using the seek functions because of the \r handling. Signed-off-by: Claudio Bley <cbley@av-test.de> --- src/fdstream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fdstream.c b/src/fdstream.c index 10c7c2a..f7dae39 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -593,7 +593,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o", st, path, oflags, offset, length, mode); - oflags |= O_NOCTTY; + oflags |= O_NOCTTY | O_BINARY; if (oflags & O_CREAT) fd = open(path, oflags, mode); -- 1.8.3.1
On Tue, Sep 24, 2013 at 11:57:38AM +0200, Claudio Bley wrote:
On win32, using text mode for binary files might result in short reads since ASCII character 0x1A is interpreted as EOF. Also, it could lead to problems using the seek functions because of the \r handling.
Signed-off-by: Claudio Bley <cbley@av-test.de>
What did you do to hit this problem ? AFAIK the virFDStream code isn't useful on Windows at all, since it requires the iohelper process and we haven't got that working on Windows. 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 :|
At Tue, 24 Sep 2013 11:16:39 +0100, Daniel P. Berrange wrote:
On Tue, Sep 24, 2013 at 11:57:38AM +0200, Claudio Bley wrote:
On win32, using text mode for binary files might result in short reads since ASCII character 0x1A is interpreted as EOF. Also, it could lead to problems using the seek functions because of the \r handling.
Signed-off-by: Claudio Bley <cbley@av-test.de>
What did you do to hit this problem ? AFAIK the virFDStream code isn't useful on Windows at all, since it requires the iohelper process and we haven't got that working on Windows.
AFAIS, the iohelper is only needed for non-blocking I/O which I haven't used. Here's my code: ,---- | virConnectPtr conn = virConnectOpen("test:///default"); | | unsigned long libVer; | int ret = virConnectGetLibVersion(conn, &libVer); | | virDomainPtr domain = virDomainLookupByName(conn, "test"); | | virStreamPtr stream = virStreamNew(conn, 0); | | char* mimetype = virDomainScreenshot(domain, stream, 0, 0); | char buf[8192]; | int got = 0; | | puts(mimetype); | | do { | got = virStreamRecv(stream, buf, 8192); | | switch (got) { | case 0: | virStreamFinish(stream); | break; | | case -1: | puts("error"); | virStreamAbort(stream); | goto cleanup; | | default: | printf("got %d bytes\n", got); | } | } while(got > 0); | | cleanup: | virDomainFree(domain); | virConnectClose(conn); `---- Here's the output: 2013-09-24 07:30:46.823+0000: 3052: debug : virDomainScreenshot:3241 : dom=0000000000681D30, (VM: name=test, uuid=6695eb01-f6a4-8304-79aa-97f2502e193f), stream=0000000000681E30, flags=0 2013-09-24 07:30:46.823+0000: 3052: debug : virFDStreamOpenFileInternal:599 : st=0000000000681E30 path=/usr/x86_64-w64-mingw32/sys-root/mingw/share/libvirt/libvirtLogo.png oflags=0 offset=0 length=0 mode=0 2013-09-24 07:30:46.825+0000: 3052: debug : virFDStreamOpenInternal:490 : st=0000000000681E30 fd=3 cmd=0000000000000000 errfd=-1 length=0 image/png 2013-09-24 07:30:46.826+0000: 3052: debug : virStreamRecv:17138 : stream=0000000000681E30, data=000000000022DE20, nbytes=8192 got 5 bytes 2013-09-24 07:30:46.828+0000: 3052: debug : virStreamRecv:17138 : stream=0000000000681E30, data=000000000022DE20, nbytes=8192 2013-09-24 07:30:46.830+0000: 3052: debug : virStreamFinish:17506 : stream=0000000000681E30 2013-09-24 07:30:46.831+0000: 3052: debug : virFDStreamCloseInt:260 : st=0000000000681E30 2013-09-24 07:30:46.831+0000: 3052: debug : virFileClose:90 : Closed fd 3 / Claudio -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On 09/24/2013 03:57 AM, Claudio Bley wrote:
On win32, using text mode for binary files might result in short reads since ASCII character 0x1A is interpreted as EOF. Also, it could lead to problems using the seek functions because of the \r handling.
Signed-off-by: Claudio Bley <cbley@av-test.de> --- src/fdstream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
diff --git a/src/fdstream.c b/src/fdstream.c index 10c7c2a..f7dae39 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -593,7 +593,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o", st, path, oflags, offset, length, mode);
- oflags |= O_NOCTTY; + oflags |= O_NOCTTY | O_BINARY;
if (oflags & O_CREAT) fd = open(path, oflags, mode);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Claudio Bley -
Daniel P. Berrange -
Eric Blake