10:04 a.m.
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
10:04 a.m.
New subject: [PATCH 1/6] qemu: fix domain start with corrupted save file
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(a)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
11:22 a.m.
New subject: [PATCH 1/6] qemu: fix domain start with corrupted save file
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(a)redhat.com>
Reviewed-by: Pavel Mores <pmores(a)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
11:41 a.m.
New subject: [PATCH 1/6] qemu: fix domain start with corrupted save file
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(a)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(a)redhat.com>
Jano
10:04 a.m.
New subject: [PATCH 2/6] virQEMUSaveData: Register autoclear function and use it in qemuDomainSaveImageOpen
In an attempt to simplify qemuDomainSaveImageOpen we need to add
automatic pointer clearing for virQEMUSaveData.
Signed-off-by: Peter Krempa <pkrempa(a)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
11:49 a.m.
New subject: [PATCH 2/6] virQEMUSaveData: Register autoclear function and use it in qemuDomainSaveImageOpen
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(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:04 a.m.
New subject: [PATCH 3/6] qemuDomainSaveImageOpen: Use g_autoptr for 'dom'
Signed-off-by: Peter Krempa <pkrempa(a)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
11:51 a.m.
New subject: [PATCH 3/6] qemuDomainSaveImageOpen: Use g_autoptr for 'dom'
s/dom/def in the commit message
On a Wednesday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Proofread-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:04 a.m.
New subject: [PATCH 4/6] qemuDomainSaveImageOpen: Automatically close 'fd' if unneeded
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(a)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
11:52 a.m.
New subject: [PATCH 4/6] qemuDomainSaveImageOpen: Automatically close 'fd' if unneeded
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(a)redhat.com>
---
src/qemu/qemu_driver.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:04 a.m.
New subject: [PATCH 5/6] qemuDomainSaveImageOpen: Use 'g_new0' instead of VIR_ALLOC(_N)
Signed-off-by: Peter Krempa <pkrempa(a)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
11:53 a.m.
New subject: [PATCH 5/6] qemuDomainSaveImageOpen: Use 'g_new0' instead of VIR_ALLOC(_N)
On a Wednesday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:04 a.m.
New subject: [PATCH 6/6] qemuDomainSaveImageOpen: Refactor handling of errors
Return error codes directly and fix weird reporting of errors via
temporary variable.
Signed-off-by: Peter Krempa <pkrempa(a)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
11:53 a.m.
New subject: [PATCH 6/6] qemuDomainSaveImageOpen: Refactor handling of errors
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(a)redhat.com>
---
src/qemu/qemu_driver.c | 51 +++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 25 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1675
days inactive
1675
days old
13 comments
3 participants
participants (3)
-
Ján Tomko -
Pavel Mores -
Peter Krempa