[PATCH 0/6] qemuDomainSaveImageOpen: Fix startup with corrupted save image
This is an alternative to https://www.redhat.com/archives/libvir-list/2020-April/msg01081.html which actually fixes qemuDomainSaveImageOpen in a way that would make it more obvious which value needs to be returned and also cleans up the code some more. Peter Krempa (6): qemu: fix domain start with corrupted save file virQEMUSaveData: Register autoclear function and use it in qemuDomainSaveImageOpen qemuDomainSaveImageOpen: Use g_autoptr for 'dom' qemuDomainSaveImageOpen: Automatically close 'fd' if unneeded qemuDomainSaveImageOpen: Use 'g_new0' instead of VIR_ALLOC(_N) qemuDomainSaveImageOpen: Refactor handling of errors src/qemu/qemu_driver.c | 82 +++++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 41 deletions(-) -- 2.26.0
Commit 21ad56e932 introduced a regression where a VM with a corrupted save image file would fail to start on the first attempt. This was caused by returning a wrong return code as 'fd' was abused to also hold the return code. Since it's easy to miss this nuance introduce a 'ret' variable for the return code and return it's value in the error section. https://bugzilla.redhat.com/show_bug.cgi?id=1791522 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dfe0adaad8..9a9361949d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6691,6 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, bool unlink_corrupt) { int fd = -1; + int ret = -1; virQEMUSaveDataPtr data = NULL; virQEMUSaveHeaderPtr header; virDomainDefPtr def = NULL; @@ -6726,7 +6727,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, _("cannot remove corrupt file: %s"), path); } else { - fd = -3; + ret = -3; } } else { virReportError(VIR_ERR_OPERATION_FAILED, @@ -6747,7 +6748,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, _("cannot remove corrupt file: %s"), path); } else { - fd = -3; + ret = -3; } goto error; } @@ -6816,7 +6817,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, virDomainDefFree(def); virQEMUSaveDataFree(data); VIR_FORCE_CLOSE(fd); - return -1; + return ret; } static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) -- 2.26.0
On Wed, Apr 22, 2020 at 05:04:54PM +0200, Peter Krempa wrote:
Commit 21ad56e932 introduced a regression where a VM with a corrupted save image file would fail to start on the first attempt. This was caused by returning a wrong return code as 'fd' was abused to also hold the return code.
Since it's easy to miss this nuance introduce a 'ret' variable for the return code and return it's value in the error section.
https://bugzilla.redhat.com/show_bug.cgi?id=1791522
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
--- src/qemu/qemu_driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dfe0adaad8..9a9361949d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6691,6 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, bool unlink_corrupt) { int fd = -1; + int ret = -1; virQEMUSaveDataPtr data = NULL; virQEMUSaveHeaderPtr header; virDomainDefPtr def = NULL; @@ -6726,7 +6727,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, _("cannot remove corrupt file: %s"), path); } else { - fd = -3; + ret = -3; } } else { virReportError(VIR_ERR_OPERATION_FAILED, @@ -6747,7 +6748,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, _("cannot remove corrupt file: %s"), path); } else { - fd = -3; + ret = -3; } goto error; } @@ -6816,7 +6817,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, virDomainDefFree(def); virQEMUSaveDataFree(data); VIR_FORCE_CLOSE(fd); - return -1; + return ret; }
static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) -- 2.26.0
On a Wednesday in 2020, Peter Krempa wrote:
Commit 21ad56e932 introduced a regression where a VM with a corrupted save image file would fail to start on the first attempt. This was caused by returning a wrong return code as 'fd' was abused to also hold
double space
the return code.
Since it's easy to miss this nuance introduce a 'ret' variable for the
missing comma
return code and return it's value in the error section.
its
https://bugzilla.redhat.com/show_bug.cgi?id=1791522
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dfe0adaad8..9a9361949d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6691,6 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, bool unlink_corrupt) { int fd = -1; + int ret = -1; virQEMUSaveDataPtr data = NULL; virQEMUSaveHeaderPtr header; virDomainDefPtr def = NULL;
This does not restore the preservation of -errno from the following call: if ((fd = qemuOpenFile(driver, NULL, path, oflags, NULL)) < 0) goto error; that existed before commit 21ad56e932 mentioned above. Thankfully. Guess it's unlikely that this function would return -ESRCH and confuse the caller. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In an attempt to simplify qemuDomainSaveImageOpen we need to add automatic pointer clearing for virQEMUSaveData. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a9361949d..5b87aaf9c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2856,6 +2856,7 @@ virQEMUSaveDataFree(virQEMUSaveDataPtr data) VIR_FREE(data); } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUSaveData, virQEMUSaveDataFree); /** * This function steals @domXML on success. @@ -6692,7 +6693,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, { int fd = -1; int ret = -1; - virQEMUSaveDataPtr data = NULL; + g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeaderPtr header; virDomainDefPtr def = NULL; int oflags = open_write ? O_RDWR : O_RDONLY; @@ -6809,13 +6810,12 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, goto error; *ret_def = def; - *ret_data = data; + *ret_data = g_steal_pointer(&data); return fd; error: virDomainDefFree(def); - virQEMUSaveDataFree(data); VIR_FORCE_CLOSE(fd); return ret; } -- 2.26.0
On a Wednesday in 2020, Peter Krempa wrote:
In an attempt to simplify qemuDomainSaveImageOpen we need to add automatic pointer clearing for virQEMUSaveData.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5b87aaf9c2..57c66c3401 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6695,7 +6695,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeaderPtr header; - virDomainDefPtr def = NULL; + g_autoptr(virDomainDef) def = NULL; int oflags = open_write ? O_RDWR : O_RDONLY; size_t xml_len; size_t cookie_len; @@ -6809,13 +6809,12 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto error; - *ret_def = def; + *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data); return fd; error: - virDomainDefFree(def); VIR_FORCE_CLOSE(fd); return ret; } -- 2.26.0
s/dom/def in the commit message On a Wednesday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Proofread-by: Ján Tomko <jtomko@redhat.com> Jano
Use VIR_AUTOCLOSE to declare it and remove all internal closing of the filedescriptor. This will allow getting rid of 'error' completely. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57c66c3401..c0ce1583b1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6691,7 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, bool open_write, bool unlink_corrupt) { - int fd = -1; + VIR_AUTOCLOSE fd = -1; int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeaderPtr header; @@ -6723,7 +6723,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, header = &data->header; if (saferead(fd, header, sizeof(*header)) != sizeof(*header)) { if (unlink_corrupt) { - if (VIR_CLOSE(fd) < 0 || unlink(path) < 0) { + if (unlink(path) < 0) { virReportSystemError(errno, _("cannot remove corrupt file: %s"), path); @@ -6744,7 +6744,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, sizeof(header->magic)) == 0) { msg = _("save image is incomplete"); if (unlink_corrupt) { - if (VIR_CLOSE(fd) < 0 || unlink(path) < 0) { + if (unlink(path) < 0) { virReportSystemError(errno, _("cannot remove corrupt file: %s"), path); @@ -6812,10 +6812,12 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data); - return fd; + ret = fd; + fd = -1; + + return ret; error: - VIR_FORCE_CLOSE(fd); return ret; } -- 2.26.0
On a Wednesday in 2020, Peter Krempa wrote:
Use VIR_AUTOCLOSE to declare it and remove all internal closing of the filedescriptor. This will allow getting rid of 'error' completely.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c0ce1583b1..70cdbe235a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6717,8 +6717,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, VIR_FILE_WRAPPER_BYPASS_CACHE))) goto error; - if (VIR_ALLOC(data) < 0) - goto error; + data = g_new0(virQEMUSaveData, 1); header = &data->header; if (saferead(fd, header, sizeof(*header)) != sizeof(*header)) { @@ -6783,8 +6782,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, cookie_len = header->data_len - xml_len; - if (VIR_ALLOC_N(data->xml, xml_len) < 0) - goto error; + data->xml = g_new0(char, xml_len); if (saferead(fd, data->xml, xml_len) != xml_len) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -6793,8 +6791,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, } if (cookie_len > 0) { - if (VIR_ALLOC_N(data->cookie, cookie_len) < 0) - goto error; + data->cookie = g_new0(char, cookie_len); if (saferead(fd, data->cookie, cookie_len) != cookie_len) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", -- 2.26.0
On a Wednesday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Return error codes directly and fix weird reporting of errors via temporary variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 51 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 70cdbe235a..00d276061e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6705,17 +6705,18 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, if (directFlag < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("bypass cache unsupported by this system")); - goto error; + return -1; } oflags |= directFlag; } if ((fd = qemuOpenFile(driver, NULL, path, oflags, NULL)) < 0) - goto error; + return -1; + if (bypass_cache && !(*wrapperFd = virFileWrapperFdNew(&fd, path, VIR_FILE_WRAPPER_BYPASS_CACHE))) - goto error; + return -1; data = g_new0(virQEMUSaveData, 1); @@ -6726,35 +6727,38 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, virReportSystemError(errno, _("cannot remove corrupt file: %s"), path); + return -1; } else { - ret = -3; + return -3; } - } else { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("failed to read qemu header")); } - goto error; + + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("failed to read qemu header")); + return -1; } if (memcmp(header->magic, QEMU_SAVE_MAGIC, sizeof(header->magic)) != 0) { - const char *msg = _("image magic is incorrect"); - - if (memcmp(header->magic, QEMU_SAVE_PARTIAL, - sizeof(header->magic)) == 0) { - msg = _("save image is incomplete"); + if (memcmp(header->magic, QEMU_SAVE_PARTIAL, sizeof(header->magic)) == 0) { if (unlink_corrupt) { if (unlink(path) < 0) { virReportSystemError(errno, _("cannot remove corrupt file: %s"), path); + return -1; } else { - ret = -3; + return -3; } - goto error; } + + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("save image is incomplete")); + return -1; } - virReportError(VIR_ERR_OPERATION_FAILED, "%s", msg); - goto error; + + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("image magic is incorrect")); + return -1; } if (header->version > QEMU_SAVE_VERSION) { @@ -6766,13 +6770,13 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, virReportError(VIR_ERR_OPERATION_FAILED, _("image version is not supported (%d > %d)"), header->version, QEMU_SAVE_VERSION); - goto error; + return -1; } if (header->data_len <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, _("invalid header data length: %d"), header->data_len); - goto error; + return -1; } if (header->cookieOffset) @@ -6787,7 +6791,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, if (saferead(fd, data->xml, xml_len) != xml_len) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to read domain XML")); - goto error; + return -1; } if (cookie_len > 0) { @@ -6796,7 +6800,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, if (saferead(fd, data->cookie, cookie_len) != cookie_len) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to read cookie")); - goto error; + return -1; } } @@ -6804,7 +6808,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, if (!(def = virDomainDefParseString(data->xml, driver->xmlopt, qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) - goto error; + return -1; *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data); @@ -6813,9 +6817,6 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, fd = -1; return ret; - - error: - return ret; } static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) -- 2.26.0
On a Wednesday in 2020, Peter Krempa wrote:
Return error codes directly and fix weird reporting of errors via temporary variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 51 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 25 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Pavel Mores -
Peter Krempa