The buffer that allocated in the virFDStreamThreadDoRead() can be automatically freed, or if saved into the message structure it can be stolen. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 1c32be47a9..e29c95690b 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -431,7 +431,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, virFDStreamMsgPtr msg = NULL; int inData = 0; long long sectionLen = 0; - char *buf = NULL; + g_autofree char *buf = NULL; ssize_t got; if (sparse && *dataLen == 0) { @@ -483,9 +483,8 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, } msg->type = VIR_FDSTREAM_MSG_TYPE_DATA; - msg->stream.data.buf = buf; + msg->stream.data.buf = g_steal_pointer(&buf); msg->stream.data.len = got; - buf = NULL; if (sparse) *dataLen -= got; } @@ -496,7 +495,6 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, return got; error: - VIR_FREE(buf); virFDStreamMsgFree(msg); return -1; } -- 2.26.2

All callers of virFDStreamMsgQueuePush() have the same pattern: they explicitly set @msg passed to NULL to avoid freeing it later on. Well, the function can take address of the pointer and clear it for them. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index e29c95690b..6efe6c17ad 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -140,7 +140,7 @@ VIR_ONCE_GLOBAL_INIT(virFDStreamData); static int virFDStreamMsgQueuePush(virFDStreamDataPtr fdst, - virFDStreamMsgPtr msg, + virFDStreamMsgPtr *msg, int fd, const char *fdname) { @@ -150,7 +150,7 @@ virFDStreamMsgQueuePush(virFDStreamDataPtr fdst, while (*tmp) tmp = &(*tmp)->next; - *tmp = msg; + *tmp = g_steal_pointer(msg); virCondSignal(&fdst->threadCond); if (safewrite(fd, &c, sizeof(c)) != sizeof(c)) { @@ -489,8 +489,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, *dataLen -= got; } - virFDStreamMsgQueuePush(fdst, msg, fdout, fdoutname); - msg = NULL; + virFDStreamMsgQueuePush(fdst, &msg, fdout, fdoutname); return got; @@ -814,8 +813,7 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) msg->stream.data.buf = buf; msg->stream.data.len = nbytes; - virFDStreamMsgQueuePush(fdst, msg, fdst->fd, "pipe"); - msg = NULL; + virFDStreamMsgQueuePush(fdst, &msg, fdst->fd, "pipe"); ret = nbytes; } else { retry: @@ -1010,8 +1008,7 @@ virFDStreamSendHole(virStreamPtr st, msg->type = VIR_FDSTREAM_MSG_TYPE_HOLE; msg->stream.hole.len = length; - virFDStreamMsgQueuePush(fdst, msg, fdst->fd, "pipe"); - msg = NULL; + virFDStreamMsgQueuePush(fdst, &msg, fdst->fd, "pipe"); } } else { off = lseek(fdst->fd, length, SEEK_CUR); -- 2.26.2

A cleanup function can be declared for virFDStreamMsg type so that the structure doesn't have to be freed explicitly. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 6efe6c17ad..25661736ca 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -208,6 +208,8 @@ virFDStreamMsgFree(virFDStreamMsgPtr msg) VIR_FREE(msg); } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFDStreamMsg, virFDStreamMsgFree); + static void virFDStreamMsgQueueFree(virFDStreamMsgPtr *queue) @@ -428,7 +430,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, size_t *dataLen, size_t buflen) { - virFDStreamMsgPtr msg = NULL; + g_autoptr(virFDStreamMsg) msg = NULL; int inData = 0; long long sectionLen = 0; g_autofree char *buf = NULL; @@ -494,7 +496,6 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, return got; error: - virFDStreamMsgFree(msg); return -1; } @@ -761,7 +762,7 @@ virFDStreamAbort(virStreamPtr st) static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) { virFDStreamDataPtr fdst = st->privateData; - virFDStreamMsgPtr msg = NULL; + g_autoptr(virFDStreamMsg) msg = NULL; int ret = -1; if (nbytes > INT_MAX) { @@ -838,7 +839,6 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) cleanup: virObjectUnlock(fdst); - virFDStreamMsgFree(msg); return ret; } @@ -960,7 +960,7 @@ virFDStreamSendHole(virStreamPtr st, unsigned int flags) { virFDStreamDataPtr fdst = st->privateData; - virFDStreamMsgPtr msg = NULL; + g_autoptr(virFDStreamMsg) msg = NULL; off_t off; int ret = -1; @@ -1028,7 +1028,6 @@ virFDStreamSendHole(virStreamPtr st, ret = 0; cleanup: virObjectUnlock(fdst); - virFDStreamMsgFree(msg); return ret; } -- 2.26.2

This switch allow us to save a few lines of code. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 25661736ca..c85dee05c3 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -452,8 +452,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, buflen > length - total) buflen = length - total; - if (VIR_ALLOC(msg) < 0) - goto error; + msg = g_new0(virFDStreamMsg, 1); if (sparse && *dataLen == 0) { msg->type = VIR_FDSTREAM_MSG_TYPE_HOLE; @@ -474,8 +473,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, buflen > *dataLen) buflen = *dataLen; - if (VIR_ALLOC_N(buf, buflen) < 0) - goto error; + buf = g_new0(char, buflen); if ((got = saferead(fdin, buf, buflen)) < 0) { virReportSystemError(errno, @@ -805,9 +803,8 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) goto cleanup; } - if (VIR_ALLOC(msg) < 0 || - VIR_ALLOC_N(buf, nbytes) < 0) - goto cleanup; + msg = g_new0(virFDStreamMsg, 1); + buf = g_new0(char, nbytes); memcpy(buf, bytes, nbytes); msg->type = VIR_FDSTREAM_MSG_TYPE_DATA; @@ -1003,8 +1000,7 @@ virFDStreamSendHole(virStreamPtr st, virFDStreamMsgQueuePop(fdst, fdst->fd, "pipe"); } else { - if (VIR_ALLOC(msg) < 0) - goto cleanup; + msg = g_new0(virFDStreamMsg, 1); msg->type = VIR_FDSTREAM_MSG_TYPE_HOLE; msg->stream.hole.len = length; @@ -1123,8 +1119,7 @@ static int virFDStreamOpenInternal(virStreamPtr st, /* Create the thread after fdst and st were initialized. * The thread worker expects them to be that way. */ - if (VIR_ALLOC(fdst->thread) < 0) - goto error; + fdst->thread = g_new0(virThread, 1); if (virCondInit(&fdst->threadCond) < 0) { virReportSystemError(errno, "%s", @@ -1277,8 +1272,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, if (virPipe(pipefds) < 0) goto error; - if (VIR_ALLOC(threadData) < 0) - goto error; + threadData = g_new0(virFDStreamThreadData, 1); threadData->st = virObjectRef(st); threadData->length = length; -- 2.26.2

Again, instead of closing FDs explicitly, we can automatically close them when they go out of their respective scopes. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index c85dee05c3..bac1c95c0a 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -571,9 +571,9 @@ virFDStreamThread(void *opaque) virStreamPtr st = data->st; size_t length = data->length; bool sparse = data->sparse; - int fdin = data->fdin; + VIR_AUTOCLOSE fdin = data->fdin; char *fdinname = data->fdinname; - int fdout = data->fdout; + VIR_AUTOCLOSE fdout = data->fdout; char *fdoutname = data->fdoutname; virFDStreamDataPtr fdst = st->privateData; bool doRead = fdst->threadDoRead; @@ -633,8 +633,6 @@ virFDStreamThread(void *opaque) virObjectUnref(fdst); if (virFDStreamDataDisposed) st->privateData = NULL; - VIR_FORCE_CLOSE(fdin); - VIR_FORCE_CLOSE(fdout); virFDStreamThreadDataFree(data); return; @@ -1160,9 +1158,10 @@ int virFDStreamConnectUNIX(virStreamPtr st, { struct sockaddr_un sa; virTimeBackOffVar timeout; + VIR_AUTOCLOSE fd = -1; int ret; - int fd = socket(AF_UNIX, SOCK_STREAM, 0); + fd = socket(AF_UNIX, SOCK_STREAM, 0); if (fd < 0) { virReportSystemError(errno, "%s", _("Unable to open UNIX socket")); goto error; @@ -1197,10 +1196,11 @@ int virFDStreamConnectUNIX(virStreamPtr st, if (virFDStreamOpenInternal(st, fd, NULL, 0) < 0) goto error; + + fd = -1; return 0; error: - VIR_FORCE_CLOSE(fd); return -1; } -- 2.26.2

After previous cleanups, some labels in some functions have nothing but 'return' statement in them. Drop the labels and replace 'goto'-s with respective return statements. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index bac1c95c0a..50209a8bd4 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -438,7 +438,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, if (sparse && *dataLen == 0) { if (virFileInData(fdin, &inData, &sectionLen) < 0) - goto error; + return -1; if (length && sectionLen > length - total) @@ -466,7 +466,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, virReportSystemError(errno, _("unable to seek in %s"), fdinname); - goto error; + return -1; } } else { if (sparse && @@ -479,7 +479,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, virReportSystemError(errno, _("Unable to read %s"), fdinname); - goto error; + return -1; } msg->type = VIR_FDSTREAM_MSG_TYPE_DATA; @@ -492,9 +492,6 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, virFDStreamMsgQueuePush(fdst, &msg, fdout, fdoutname); return got; - - error: - return -1; } @@ -1164,22 +1161,22 @@ int virFDStreamConnectUNIX(virStreamPtr st, fd = socket(AF_UNIX, SOCK_STREAM, 0); if (fd < 0) { virReportSystemError(errno, "%s", _("Unable to open UNIX socket")); - goto error; + return -1; } memset(&sa, 0, sizeof(sa)); sa.sun_family = AF_UNIX; if (abstract) { if (virStrcpy(sa.sun_path+1, path, sizeof(sa.sun_path)-1) < 0) - goto error; + return -1; sa.sun_path[0] = '\0'; } else { if (virStrcpyStatic(sa.sun_path, path) < 0) - goto error; + return -1; } if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0) - goto error; + return -1; while (virTimeBackOffWait(&timeout)) { ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa)); if (ret == 0) @@ -1191,17 +1188,14 @@ int virFDStreamConnectUNIX(virStreamPtr st, continue; } - goto error; + return -1; } if (virFDStreamOpenInternal(st, fd, NULL, 0) < 0) - goto error; + return -1; fd = -1; return 0; - - error: - return -1; } -- 2.26.2

For libvirt, the volume is just a binary blob and it doesn't interpret data on volume upload/download. But as it turns out, this unspoken assumption is not clear to our users. Document it explicitly. Suggested in: https://bugzilla.redhat.com/show_bug.cgi?id=1851023#c17 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt-storage.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c index a45c8b98c1..8738f6dd14 100644 --- a/src/libvirt-storage.c +++ b/src/libvirt-storage.c @@ -1590,7 +1590,9 @@ virStorageVolCreateXMLFrom(virStoragePoolPtr pool, * * Download the content of the volume as a stream. If @length * is zero, then the remaining contents of the volume after - * @offset will be downloaded. + * @offset will be downloaded. Please note, that the data is + * not interpreted and therefore data received by stream + * client are in the very same format the volume is in. * * If VIR_STORAGE_VOL_DOWNLOAD_SPARSE_STREAM is set in @flags * effective transmission of holes is enabled. This assumes using @@ -1663,7 +1665,9 @@ virStorageVolDownload(virStorageVolPtr vol, * will fail if @offset + @length exceeds the size of the * volume. Otherwise, if @length is non-zero, an error * will be raised if an attempt is made to upload greater - * than @length bytes of data. + * than @length bytes of data. Please note, that the data is + * not interpreted and therefore data sent by stream client + * must be in the very same format the volume is in. * * If VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM is set in @flags * effective transmission of holes is enabled. This assumes using -- 2.26.2

On 8/20/20 10:35 AM, Peter Krempa wrote: Fine by me. Michal

This function will be used to detect zero buffers (which are going to be interpreted as hole in virStream later). I shamelessly took inspiration from coreutils. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virstring.c | 38 ++++++++++++++++++++++++++++++++ src/util/virstring.h | 2 ++ tests/virstringtest.c | 47 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ae0e253ab9..1d80aeb833 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3197,6 +3197,7 @@ virStringHasChars; virStringHasControlChars; virStringHasSuffix; virStringIsEmpty; +virStringIsNull; virStringIsPrintable; virStringListAdd; virStringListAutoFree; diff --git a/src/util/virstring.c b/src/util/virstring.c index e9e792f3bf..c26bc770d4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1404,3 +1404,41 @@ int virStringParseYesNo(const char *str, bool *result) return 0; } + + +/** + * virStringIsNull: + * @buf: buffer to check + * @len: the length of the buffer + * + * For given buffer @buf and its size @len determine whether + * it contains only zero bytes (NUL) or not. + * + * Returns: true if buffer is full of zero bytes, + * false otherwise. + */ +bool virStringIsNull(const char *buf, size_t len) +{ + const char *p = buf; + + if (!len) + return true; + + /* Check up to 16 first bytes. */ + for (;;) { + if (*p) + return false; + + p++; + len--; + + if (!len) + return true; + + if ((len & 0xf) == 0) + break; + } + + /* Now we know first 16 bytes are NUL, memcmp with self. */ + return memcmp(buf, p, len) == 0; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index 360c68395c..d0e54358ac 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -185,6 +185,8 @@ int virStringParsePort(const char *str, int virStringParseYesNo(const char *str, bool *result) G_GNUC_WARN_UNUSED_RESULT; +bool virStringIsNull(const char *buf, size_t len); + /** * VIR_AUTOSTRINGLIST: * diff --git a/tests/virstringtest.c b/tests/virstringtest.c index bee49e6cb6..5838a57574 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -649,6 +649,27 @@ static int testFilterChars(const void *args) return ret; } +struct testIsNulData { + const char *buf; + size_t len; + bool nul; +}; + +static int +testIsNul(const void *args) +{ + const struct testIsNulData *data = args; + int rc; + + rc = virStringIsNull(data->buf, data->len); + if (rc != data->nul) { + fprintf(stderr, "Returned %d, expected %d\n", rc, data->nul); + return -1; + } + + return 0; +} + static int mymain(void) { @@ -977,6 +998,32 @@ mymain(void) TEST_FILTER_CHARS(NULL, NULL, NULL); TEST_FILTER_CHARS("hello 123 hello", "helo", "hellohello"); +#define TEST_IS_NUL(expect, ...) \ + do { \ + const char buf[] = {__VA_ARGS__ }; \ + struct testIsNulData isNulData = { \ + .buf = buf, \ + .len = G_N_ELEMENTS(buf), \ + .nul = expect \ + }; \ + if (virTestRun("isNul check", \ + testIsNul, &isNulData) < 0) \ + ret = -1; \ + } while (0) + + TEST_IS_NUL(true); + TEST_IS_NUL(true, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00); + TEST_IS_NUL(false, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01); + TEST_IS_NUL(false, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01); + TEST_IS_NUL(false, + 0x00, 0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x01, + 0x00, 0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x01, + 0x00); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.26.2

On Thu, Aug 20, 2020 at 01:02:55PM +0200, Peter Krempa wrote: Depends whether we care about this sparsification having goood performance or not. As a point of reference, QEMU has invested tonnes of effort in its impl, using highly tuned impls for SSE, AVX, etc switched at runtime. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/20/20 1:02 PM, Peter Krempa wrote: Sure. But it was discussed in v1 and I think we agreed that the algorithm is generic enough that it can be used in Libvirt too: https://www.redhat.com/archives/libvir-list/2020-July/msg00170.html Fair enough. I'm out of ideas though. Do you have a suggestion? Alright. I will try to find better location. But since I want to use this function from virsh too I'd like to have it in utils. The idea is to pass aligned address to memcmp(). But I guess we can let memcmp() deal with that. Michal

For given file descriptor determine if the current position it is in plus 1MiB (arbitrary chosen value) consists solely from zero bytes or not. This is a block device friendly version of virFileInData(). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virfile.c | 67 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 4 +++ 3 files changed, 72 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1d80aeb833..6b5a751788 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2070,6 +2070,7 @@ virFileGetMountSubtree; virFileGetXAttr; virFileGetXAttrQuiet; virFileInData; +virFileInDataDetectZeroes; virFileIsCDROM; virFileIsDir; virFileIsExecutable; diff --git a/src/util/virfile.c b/src/util/virfile.c index c034df5931..a35d9ccb7a 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4064,6 +4064,73 @@ virFileInData(int fd G_GNUC_UNUSED, #endif /* !HAVE_DECL_SEEK_HOLE */ +#define DETECT_ZEORES_BLOCK_SIZE (1 * 1024 * 1024) + +/* + * virFileInDataDetectZeroes: + * @fd: file to check + * @inData: true if current position in the @fd is in data section + * @length: amount of bytes until the end of the current section + * + * This behaves exactly like virFileInData() except it doesn't use SEEK_DATA + * and SEEK_HOLE rather than a buffer into which it reads data from @fd and + * detects if the buffer is full of zeroes. Therefore, it is safe to use on + * special files (e.g. block devices). On the other hand it sees only + * DETECT_ZEORES_BLOCK_SIZE ahead. + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +virFileInDataDetectZeroes(int fd, + int *inData, + long long *length) +{ + const size_t buflen = DETECT_ZEORES_BLOCK_SIZE; + g_autofree char *bytes = NULL; + off_t cur; + ssize_t r; + int ret = -1; + + /* Get current position */ + cur = lseek(fd, 0, SEEK_CUR); + if (cur == (off_t) -1) { + virReportSystemError(errno, "%s", + _("Unable to get current position in file")); + goto cleanup; + } + + bytes = g_new0(char, buflen); + + if ((r = saferead(fd, bytes, buflen)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to read from file")); + goto cleanup; + } + + *inData = !virStringIsNull(bytes, r); + *length = r; + ret = 0; + + cleanup: + /* At any rate, reposition back to where we started. */ + if (cur != (off_t) -1) { + int theerrno = errno; + + if (lseek(fd, cur, SEEK_SET) == (off_t) -1) { + virReportSystemError(errno, "%s", + _("unable to restore position in file")); + ret = -1; + if (theerrno == 0) + theerrno = errno; + } + + errno = theerrno; + } + return ret; +} + + /** * virFileReadValueInt: * @value: pointer to int to be filled in with the value diff --git a/src/util/virfile.h b/src/util/virfile.h index 7a92364a5c..9a5eade609 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -352,6 +352,10 @@ int virFileInData(int fd, int *inData, long long *length); +int virFileInDataDetectZeroes(int fd, + int *inData, + long long *length); + G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFileWrapperFd, virFileWrapperFdFree); int virFileGetXAttr(const char *path, -- 2.26.2

On Fri, Aug 21, 2020 at 11:19:40 +0200, Michal Privoznik wrote: Well, for a substantial change like this it should be the default approach. You can use: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; Don't forget to send the final version to the list though.

This is similar to one of previous patches. When receiving stream (on virStorageVolUpload() and subsequent virStreamSparseSendAll()) we may receive a hole. If the volume we are saving the incoming data into is a regular file we just lseek() and ftruncate() to create the hole. But this won't work if the file is a block device. If that is the case we must write zeroes so that any subsequent reader reads nothing just zeroes (just like they would from a hole in a regular file). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 59 +++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 15 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 44b4855549..40825a8811 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -505,6 +505,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, static ssize_t virFDStreamThreadDoWrite(virFDStreamDataPtr fdst, bool sparse, + bool isBlock, const int fdin, const int fdout, const char *fdinname, @@ -512,7 +513,6 @@ virFDStreamThreadDoWrite(virFDStreamDataPtr fdst, { ssize_t got = 0; virFDStreamMsgPtr msg = fdst->msg; - off_t off; bool pop = false; switch (msg->type) { @@ -540,19 +540,48 @@ virFDStreamThreadDoWrite(virFDStreamDataPtr fdst, } got = msg->stream.hole.len; - off = lseek(fdout, got, SEEK_CUR); - if (off == (off_t) -1) { - virReportSystemError(errno, - _("unable to seek in %s"), - fdoutname); - return -1; - } - - if (ftruncate(fdout, off) < 0) { - virReportSystemError(errno, - _("unable to truncate %s"), - fdoutname); - return -1; + if (isBlock) { + g_autofree char * buf = NULL; + const size_t buflen = 1 * 1024 * 1024; /* 1MiB */ + size_t toWrite = got; + + /* While for files it's enough to lseek() and ftruncate() to create + * a hole which would emulate zeroes on read(), for block devices + * we have to write zeroes to read() zeroes. And we have to write + * @got bytes of zeroes. Do that in smaller chunks though.*/ + + buf = g_new0(char, buflen); + + while (toWrite) { + size_t count = MIN(toWrite, buflen); + ssize_t r; + + if ((r = safewrite(fdout, buf, count)) < 0) { + virReportSystemError(errno, + _("Unable to write %s"), + fdoutname); + return -1; + } + + toWrite -= r; + } + } else { + off_t off; + + off = lseek(fdout, got, SEEK_CUR); + if (off == (off_t) -1) { + virReportSystemError(errno, + _("unable to seek in %s"), + fdoutname); + return -1; + } + + if (ftruncate(fdout, off) < 0) { + virReportSystemError(errno, + _("unable to truncate %s"), + fdoutname); + return -1; + } } pop = true; @@ -618,7 +647,7 @@ virFDStreamThread(void *opaque) length, total, &dataLen, buflen); else - got = virFDStreamThreadDoWrite(fdst, sparse, + got = virFDStreamThreadDoWrite(fdst, sparse, isBlock, fdin, fdout, fdinname, fdoutname); -- 2.26.2

The aim of virFileInDataDetectZeroes() is to mimic virFileInData() so that it can be used in sparse virStream with very little change to logic. This was implemented in previous commits. These two functions act alike - the passed FD is rewound back to the position it was in when either of the functions was called. For regular files (when virFileInData()) is used, this is not a problem - rewinding an FD is usually cheap and data is read() only from data section and it's read only once. But this is not the case for virFileInDataDetectZeroes(), which doesn't use cheap lseek() to learn data/hole sections, but uses read() and zero buffer detection. Worst case scenario, virFileInDataDetectZeroes() reads 1MiB of bytes, finds it is non-zero data, rewinds FD that 1MiB back only so that virFDStreamThreadDoRead() (in case of daemon) or virshStreamSource() (in case of client) can read the same data again. Possibly, this is not that big problem because kernel is likely to keep the data still in cache. But let's not rely on that and store the buffer for later use if we learned it contains actual data. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfdstream.c | 27 +++++++++++++--------- src/util/virfile.c | 51 ++++++++++++++---------------------------- src/util/virfile.h | 3 ++- tools/virsh-util.c | 40 +++++++++++++++++++++++++++++---- tools/virsh-util.h | 3 +++ tools/virsh-volume.c | 3 ++- 6 files changed, 76 insertions(+), 51 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 40825a8811..9d617c71da 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -440,7 +440,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, if (sparse && *dataLen == 0) { if (isBlock) { - if (virFileInDataDetectZeroes(fdin, &inData, &sectionLen) < 0) + if (virFileInDataDetectZeroes(fdin, &inData, &sectionLen, &buf) < 0) return -1; } else { if (virFileInData(fdin, &inData, &sectionLen) < 0) @@ -468,7 +468,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, /* HACK: The message queue is one directional. So caller * cannot make us skip the hole. Do that for them instead. */ - if (sectionLen && + if (sectionLen && !isBlock && lseek(fdin, sectionLen, SEEK_CUR) == (off_t) -1) { virReportSystemError(errno, _("unable to seek in %s"), @@ -476,17 +476,22 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, return -1; } } else { - if (sparse && - buflen > *dataLen) - buflen = *dataLen; + if (sparse && isBlock) { + /* Data already read by virFileInDataDetectZeroes() */ + got = sectionLen; + } else { + if (sparse && + buflen > *dataLen) + buflen = *dataLen; - buf = g_new0(char, buflen); + buf = g_new0(char, buflen); - if ((got = saferead(fdin, buf, buflen)) < 0) { - virReportSystemError(errno, - _("Unable to read %s"), - fdinname); - return -1; + if ((got = saferead(fdin, buf, buflen)) < 0) { + virReportSystemError(errno, + _("Unable to read %s"), + fdinname); + return -1; + } } msg->type = VIR_FDSTREAM_MSG_TYPE_DATA; diff --git a/src/util/virfile.c b/src/util/virfile.c index a35d9ccb7a..e33a1c280c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4071,12 +4071,18 @@ virFileInData(int fd G_GNUC_UNUSED, * @fd: file to check * @inData: true if current position in the @fd is in data section * @length: amount of bytes until the end of the current section + * @buf: data read if in data section * - * This behaves exactly like virFileInData() except it doesn't use SEEK_DATA - * and SEEK_HOLE rather than a buffer into which it reads data from @fd and - * detects if the buffer is full of zeroes. Therefore, it is safe to use on - * special files (e.g. block devices). On the other hand it sees only - * DETECT_ZEORES_BLOCK_SIZE ahead. + * For given @fd it reads up to DETECT_ZEORES_BLOCK_SIZE bytes from it. + * If all bytes read are zero then @inData is set to 0, otherwise @inData + * is set to 1 and @buf is set to the read data. In either case, the + * number of bytes read is stored in @length. + * + * If @fd is at EOF, then this function sets @inData = 0 and @length = 0. + * + * Note, in contrast to virFileInData(), this function is safe to use on + * special files (e.g. block devices), does not rewind @fd back to its + * starting position, and sees only DETECT_ZEORES_BLOCK_SIZE bytes ahead. * * Returns: 0 on success, * -1 otherwise. @@ -4084,50 +4090,27 @@ virFileInData(int fd G_GNUC_UNUSED, int virFileInDataDetectZeroes(int fd, int *inData, - long long *length) + long long *length, + char **buf) { const size_t buflen = DETECT_ZEORES_BLOCK_SIZE; g_autofree char *bytes = NULL; - off_t cur; ssize_t r; - int ret = -1; - - /* Get current position */ - cur = lseek(fd, 0, SEEK_CUR); - if (cur == (off_t) -1) { - virReportSystemError(errno, "%s", - _("Unable to get current position in file")); - goto cleanup; - } bytes = g_new0(char, buflen); if ((r = saferead(fd, bytes, buflen)) < 0) { virReportSystemError(errno, "%s", _("Unable to read from file")); - goto cleanup; + return -1; } *inData = !virStringIsNull(bytes, r); *length = r; - ret = 0; + if (*inData) + *buf = g_steal_pointer(&bytes); - cleanup: - /* At any rate, reposition back to where we started. */ - if (cur != (off_t) -1) { - int theerrno = errno; - - if (lseek(fd, cur, SEEK_SET) == (off_t) -1) { - virReportSystemError(errno, "%s", - _("unable to restore position in file")); - ret = -1; - if (theerrno == 0) - theerrno = errno; - } - - errno = theerrno; - } - return ret; + return 0; } diff --git a/src/util/virfile.h b/src/util/virfile.h index 9a5eade609..06ee0bd801 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -354,7 +354,8 @@ int virFileInData(int fd, int virFileInDataDetectZeroes(int fd, int *inData, - long long *length); + long long *length, + char **buf); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFileWrapperFd, virFileWrapperFdFree); diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 867f69577f..dd2f4fb6a3 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -160,8 +160,25 @@ virshStreamSource(virStreamPtr st G_GNUC_UNUSED, { virshStreamCallbackDataPtr cbData = opaque; int fd = cbData->fd; + int r; - return saferead(fd, bytes, nbytes); + if (cbData->isBlock) { + size_t bufavail = cbData->buflen - cbData->bufoff; + + r = MIN(nbytes, bufavail); + memcpy(bytes, cbData->buf + cbData->bufoff, r); + cbData->bufoff += r; + + if (cbData->bufoff == cbData->buflen) { + VIR_FREE(cbData->buf); + cbData->buflen = 0; + cbData->bufoff = 0; + } + } else { + r = saferead(fd, bytes, nbytes); + } + + return r; } @@ -172,9 +189,11 @@ virshStreamSourceSkip(virStreamPtr st G_GNUC_UNUSED, { virshStreamCallbackDataPtr cbData = opaque; int fd = cbData->fd; - off_t cur; - if ((cur = lseek(fd, offset, SEEK_CUR)) == (off_t) -1) + /* Don't seek if the @fd is a block device. It was left at + * the desired position by virshStreamInData(). */ + if (!cbData->isBlock && + (lseek(fd, offset, SEEK_CUR)) == (off_t) -1) return -1; return 0; @@ -232,10 +251,23 @@ virshStreamInData(virStreamPtr st G_GNUC_UNUSED, int fd = cbData->fd; if (cbData->isBlock) { - if (virFileInDataDetectZeroes(fd, inData, offset) < 0) { + g_autofree char *buf = NULL; + + if (virFileInDataDetectZeroes(fd, inData, offset, &buf) < 0) { vshError(ctl, "%s", _("Unable to get current position in stream")); return -1; } + + if (*inData) { + cbData->buf = g_steal_pointer(&buf); + cbData->buflen = *offset; + cbData->bufoff = 0; + } + + /* Intentionally leaving @fd in a different position than on entering + * because either we've found a hole and virshStreamSourceSkip() would + * want to seek forward, or we've read some data and would need to + * imitate seek in virshStreamSource(). Skip seeking back and forth. */ } else { if (virFileInData(fd, inData, offset) < 0) { vshError(ctl, "%s", _("Unable to get current position in stream")); diff --git a/tools/virsh-util.h b/tools/virsh-util.h index 9ef28cfe0a..b2b5d6bcd2 100644 --- a/tools/virsh-util.h +++ b/tools/virsh-util.h @@ -73,6 +73,9 @@ struct _virshStreamCallbackData { vshControl *ctl; int fd; bool isBlock; + char *buf; + size_t buflen; + long long bufoff; }; int diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 20823e2d10..9fd1674a9f 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -678,7 +678,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) unsigned long long offset = 0, length = 0; virshControlPtr priv = ctl->privData; unsigned int flags = 0; - virshStreamCallbackData cbData; + virshStreamCallbackData cbData = { 0 }; struct stat sb; if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0) @@ -752,6 +752,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) virStorageVolFree(vol); if (st) virStreamFree(st); + VIR_FREE(cbData.buf); VIR_FORCE_CLOSE(fd); return ret; } -- 2.26.2