[libvirt] [PATCH 0/4] test_driver: implement virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML
While implementing virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML for the test driver, I realized that there exists already code for saving and loading test images which can be reused. However, it needed to be extracted from testDomainSaveFlags and testDomainRestoreFlags into separate functions. The new functions are inspired by the corresponding QEMU driver code where e.g. qemuDomainSaveImageOpen serves as a helper used by other functions. This series of patches initially extracts the code mentioned above into separate functions and then provides the test driver with implementations for virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML which make use of the newly introduced functions. Ilias Stamatis (4): test_driver: extract image saving code into a separate function test_driver: extract image loading code into a separate function test_driver: implement virDomainSaveImageDefineXML test_driver: implement virDomainSaveImageGetXMLDesc src/test/test_driver.c | 281 ++++++++++++++++++++++++++++------------- 1 file changed, 193 insertions(+), 88 deletions(-) -- 2.21.0
Extracting the code logic for writing a test image to disk from testDomainSaveFlags into a separate function, allows us to reuse this code in other functions such as testDomainSaveImageDefineXML. Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 114 +++++++++++++++++++++++++---------------- 1 file changed, 69 insertions(+), 45 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..e71b931790 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1974,75 +1974,106 @@ testDomainGetTime(virDomainPtr dom ATTRIBUTE_UNUSED, #define TEST_SAVE_MAGIC "TestGuestMagic" -static int -testDomainSaveFlags(virDomainPtr domain, const char *path, - const char *dxml, unsigned int flags) + +/** + * testDomainSaveImageWrite: + * @driver: test driver data + * @def: domain definition whose XML will be stored in the image + * @path: path of the save image + * + * Returns true on success, else false. + */ +static bool +testDomainSaveImageWrite(testDriverPtr driver, + virDomainDefPtr def, + const char *path) { - testDriverPtr privconn = domain->conn->privateData; - int fd = -1; int len; - virDomainObjPtr privdom; - virObjectEventPtr event = NULL; - int ret = -1; + int fd = -1; VIR_AUTOFREE(char *) xml = NULL; - virCheckFlags(0, -1); - if (dxml) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("xml modification unsupported")); - return -1; - } - - - if (!(privdom = testDomObjFromDomain(domain))) - goto cleanup; - - if (virDomainObjCheckActive(privdom) < 0) - goto cleanup; - - xml = virDomainDefFormat(privdom->def, privconn->caps, - VIR_DOMAIN_DEF_FORMAT_SECURE); + xml = virDomainDefFormat(def, driver->caps, VIR_DOMAIN_DEF_FORMAT_SECURE); if (xml == NULL) { virReportSystemError(errno, _("saving domain '%s' failed to allocate space for metadata"), - domain->name); - goto cleanup; + def->name); + goto error; } if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { virReportSystemError(errno, _("saving domain '%s' to '%s': open failed"), - domain->name, path); - goto cleanup; + def->name, path); + goto error; } - len = strlen(xml); + if (safewrite(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) < 0) { virReportSystemError(errno, _("saving domain '%s' to '%s': write failed"), - domain->name, path); - goto cleanup; + def->name, path); + goto error; } + + len = strlen(xml); if (safewrite(fd, (char*)&len, sizeof(len)) < 0) { virReportSystemError(errno, _("saving domain '%s' to '%s': write failed"), - domain->name, path); - goto cleanup; + def->name, path); + goto error; } + if (safewrite(fd, xml, len) < 0) { virReportSystemError(errno, _("saving domain '%s' to '%s': write failed"), - domain->name, path); - goto cleanup; + def->name, path); + goto error; } if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("saving domain '%s' to '%s': write failed"), - domain->name, path); - goto cleanup; + def->name, path); + goto error; } - fd = -1; + + return true; + + error: + /* Don't report failure in close or unlink, because + * in either case we're already in a failure scenario + * and have reported an earlier error */ + VIR_FORCE_CLOSE(fd); + unlink(path); + + return false; +} + +static int +testDomainSaveFlags(virDomainPtr domain, const char *path, + const char *dxml, unsigned int flags) +{ + testDriverPtr privconn = domain->conn->privateData; + virDomainObjPtr privdom; + virObjectEventPtr event = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (dxml) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("xml modification unsupported")); + return -1; + } + + if (!(privdom = testDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainObjCheckActive(privdom) < 0) + goto cleanup; + + if (!testDomainSaveImageWrite(privconn, privdom->def, path)) + goto cleanup; testDomainShutdownState(domain, privdom, VIR_DOMAIN_SHUTOFF_SAVED); event = virDomainEventLifecycleNewFromObj(privdom, @@ -2054,13 +2085,6 @@ testDomainSaveFlags(virDomainPtr domain, const char *path, ret = 0; cleanup: - /* Don't report failure in close or unlink, because - * in either case we're already in a failure scenario - * and have reported an earlier error */ - if (ret != 0) { - VIR_FORCE_CLOSE(fd); - unlink(path); - } virDomainObjEndAPI(&privdom); virObjectEventStateQueue(privconn->eventState, event); return ret; -- 2.21.0
On Wed, May 29, 2019 at 02:22:56PM +0200, Ilias Stamatis wrote:
Extracting the code logic for writing a test image to disk from testDomainSaveFlags into a separate function, allows us to reuse this code in other functions such as testDomainSaveImageDefineXML.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 114 +++++++++++++++++++++++++---------------- 1 file changed, 69 insertions(+), 45 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..e71b931790 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1974,75 +1974,106 @@ testDomainGetTime(virDomainPtr dom ATTRIBUTE_UNUSED,
#define TEST_SAVE_MAGIC "TestGuestMagic"
-static int -testDomainSaveFlags(virDomainPtr domain, const char *path, - const char *dxml, unsigned int flags) + +/** + * testDomainSaveImageWrite: + * @driver: test driver data + * @def: domain definition whose XML will be stored in the image + * @path: path of the save image + * + * Returns true on success, else false. + */ +static bool
A minor nitpick, I'd probably prefer 'int' rather than 'bool', feels more natural for the given function. Reviewed-by: Erik Skultety <eskultet@redhat.com>
+testDomainSaveImageWrite(testDriverPtr driver, + virDomainDefPtr def, + const char *path) {
On Mon, Jun 3, 2019 at 10:01 AM Erik Skultety <eskultet@redhat.com> wrote:
On Wed, May 29, 2019 at 02:22:56PM +0200, Ilias Stamatis wrote:
Extracting the code logic for writing a test image to disk from testDomainSaveFlags into a separate function, allows us to reuse this code in other functions such as testDomainSaveImageDefineXML.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 114 +++++++++++++++++++++++++---------------- 1 file changed, 69 insertions(+), 45 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..e71b931790 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1974,75 +1974,106 @@ testDomainGetTime(virDomainPtr dom ATTRIBUTE_UNUSED,
#define TEST_SAVE_MAGIC "TestGuestMagic"
-static int -testDomainSaveFlags(virDomainPtr domain, const char *path, - const char *dxml, unsigned int flags) + +/** + * testDomainSaveImageWrite: + * @driver: test driver data + * @def: domain definition whose XML will be stored in the image + * @path: path of the save image + * + * Returns true on success, else false. + */ +static bool
A minor nitpick, I'd probably prefer 'int' rather than 'bool', feels more natural for the given function.
I was skeptical about the return value type tbh. Initially I had it as an int as well because I wasn't sure if the bool type is used in general. But probably you're right, int makes more sense since this function could potentially return different error codes for different error cases.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
+testDomainSaveImageWrite(testDriverPtr driver, + virDomainDefPtr def, + const char *path) {
Extracting the code logic for opening and parsing a test image from testDomainRestoreFlags into a separate function, allows us to reuse this code in other functions such as testDomainSaveImageGetXMLDesc. Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 115 ++++++++++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 43 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e71b931790..ce32570b75 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2049,6 +2049,77 @@ testDomainSaveImageWrite(testDriverPtr driver, return false; } +/** + * testDomainSaveImageOpen: + * @driver: test driver data + * @path: path of the save image + * @ret_def: returns domain definition created from the XML stored in the image + * + * Returns the opened fd of the save image file and fills ret_def on success. + * Returns -1, on error. + */ +static int ATTRIBUTE_NONNULL(3) +testDomainSaveImageOpen(testDriverPtr driver, + const char *path, + virDomainDefPtr *ret_def) +{ + char magic[15]; + int fd = -1; + int len; + virDomainDefPtr def = NULL; + VIR_AUTOFREE(char *) xml = NULL; + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportSystemError(errno, _("cannot read domain image '%s'"), path); + goto error; + } + + if (saferead(fd, magic, sizeof(magic)) != sizeof(magic)) { + virReportSystemError(errno, _("incomplete save header in '%s'"), path); + goto error; + } + + if (memcmp(magic, TEST_SAVE_MAGIC, sizeof(magic))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mismatched header magic")); + goto error; + } + + if (saferead(fd, (char*)&len, sizeof(len)) != sizeof(len)) { + virReportSystemError(errno, + _("failed to read metadata length in '%s'"), + path); + goto error; + } + + if (len < 1 || len > 8192) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("length of metadata out of range")); + goto error; + } + + if (VIR_ALLOC_N(xml, len+1) < 0) + goto error; + + if (saferead(fd, xml, len) != len) { + virReportSystemError(errno, _("incomplete metadata in '%s'"), path); + goto error; + } + xml[len] = '\0'; + + if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + goto error; + + VIR_STEAL_PTR(*ret_def, def); + return fd; + + error: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + return -1; +} + static int testDomainSaveFlags(virDomainPtr domain, const char *path, const char *dxml, unsigned int flags) @@ -2104,14 +2175,11 @@ testDomainRestoreFlags(virConnectPtr conn, unsigned int flags) { testDriverPtr privconn = conn->privateData; - char magic[15]; int fd = -1; - int len; virDomainDefPtr def = NULL; virDomainObjPtr dom = NULL; virObjectEventPtr event = NULL; int ret = -1; - VIR_AUTOFREE(char *) xml = NULL; virCheckFlags(0, -1); if (dxml) { @@ -2120,46 +2188,7 @@ testDomainRestoreFlags(virConnectPtr conn, return -1; } - if ((fd = open(path, O_RDONLY)) < 0) { - virReportSystemError(errno, - _("cannot read domain image '%s'"), - path); - goto cleanup; - } - if (saferead(fd, magic, sizeof(magic)) != sizeof(magic)) { - virReportSystemError(errno, - _("incomplete save header in '%s'"), - path); - goto cleanup; - } - if (memcmp(magic, TEST_SAVE_MAGIC, sizeof(magic))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("mismatched header magic")); - goto cleanup; - } - if (saferead(fd, (char*)&len, sizeof(len)) != sizeof(len)) { - virReportSystemError(errno, - _("failed to read metadata length in '%s'"), - path); - goto cleanup; - } - if (len < 1 || len > 8192) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("length of metadata out of range")); - goto cleanup; - } - if (VIR_ALLOC_N(xml, len+1) < 0) - goto cleanup; - if (saferead(fd, xml, len) != len) { - virReportSystemError(errno, - _("incomplete metadata in '%s'"), path); - goto cleanup; - } - xml[len] = '\0'; - - def = virDomainDefParseString(xml, privconn->caps, privconn->xmlopt, - NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE); - if (!def) + if ((fd = testDomainSaveImageOpen(privconn, path, &def)) < 0) goto cleanup; if (testDomainGenerateIfnames(def) < 0) -- 2.21.0
On Wed, May 29, 2019 at 02:22:57PM +0200, Ilias Stamatis wrote:
Extracting the code logic for opening and parsing a test image from testDomainRestoreFlags into a separate function, allows us to reuse this code in other functions such as testDomainSaveImageGetXMLDesc.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 115 ++++++++++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 43 deletions(-)
This diff from format-patch seems okay, but when you apply the patch and use git show (even with -M), the diff is very hard to read. It would be improved if you extracted it right above testDomainRestoreFlags which is the only function making use of this helper. Code-wise I don't see anything wrong. Erik
On Mon, Jun 03, 2019 at 10:39:22AM +0200, Erik Skultety wrote:
On Wed, May 29, 2019 at 02:22:57PM +0200, Ilias Stamatis wrote:
Extracting the code logic for opening and parsing a test image from testDomainRestoreFlags into a separate function, allows us to reuse this code in other functions such as testDomainSaveImageGetXMLDesc.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 115 ++++++++++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 43 deletions(-)
This diff from format-patch seems okay, but when you apply the patch and use git show (even with -M), the diff is very hard to read. It would be improved if
Aaand you can disregard ^this, I have diff.algorithm set to patience in my gitconfig, so that's why. Erik
you extracted it right above testDomainRestoreFlags which is the only function making use of this helper.
Code-wise I don't see anything wrong.
Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 29, 2019 at 02:22:57PM +0200, Ilias Stamatis wrote:
Extracting the code logic for opening and parsing a test image from testDomainRestoreFlags into a separate function, allows us to reuse this code in other functions such as testDomainSaveImageGetXMLDesc.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
Updates the existing image stored in @path, in case @dxml contains valid XML supported by the fake host. Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce32570b75..b0195ac63d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2228,6 +2228,33 @@ testDomainRestore(virConnectPtr conn, return testDomainRestoreFlags(conn, path, NULL, 0); } +static int +testDomainSaveImageDefineXML(virConnectPtr conn, + const char *path, + const char *dxml, + unsigned int flags) +{ + int ret = -1; + virDomainDefPtr def = NULL; + testDriverPtr privconn = conn->privateData; + + virCheckFlags(VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); + + if ((def = virDomainDefParseString(dxml, privconn->caps, privconn->xmlopt, NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE)) == NULL) + goto cleanup; + + if (!testDomainSaveImageWrite(privconn, def, path)) + goto cleanup; + + ret = 0; + + cleanup: + virDomainDefFree(def); + return ret; +} + static int testDomainCoreDumpWithFormat(virDomainPtr domain, const char *to, unsigned int dumpformat, @@ -7010,6 +7037,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainSaveFlags = testDomainSaveFlags, /* 0.9.4 */ .domainRestore = testDomainRestore, /* 0.3.2 */ .domainRestoreFlags = testDomainRestoreFlags, /* 0.9.4 */ + .domainSaveImageDefineXML = testDomainSaveImageDefineXML, /* 5.4.0 */ .domainCoreDump = testDomainCoreDump, /* 0.3.2 */ .domainCoreDumpWithFormat = testDomainCoreDumpWithFormat, /* 1.2.3 */ .domainSetVcpus = testDomainSetVcpus, /* 0.1.4 */ -- 2.21.0
On Wed, May 29, 2019 at 02:22:58PM +0200, Ilias Stamatis wrote:
Updates the existing image stored in @path, in case @dxml contains valid XML supported by the fake host.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce32570b75..b0195ac63d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2228,6 +2228,33 @@ testDomainRestore(virConnectPtr conn, return testDomainRestoreFlags(conn, path, NULL, 0); }
+static int +testDomainSaveImageDefineXML(virConnectPtr conn, + const char *path, + const char *dxml, + unsigned int flags) +{ + int ret = -1; + virDomainDefPtr def = NULL; + testDriverPtr privconn = conn->privateData; + + virCheckFlags(VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); +
In essence I agree. I'm just wondering now that you extracted the common code in the previous patches whether we should come one step closer to how the real API behaves, IOW if the image given by @path doesn't exist, we naturally fail to open it, the current approach within the test driver is that you parse @dxml, open the image and write the new XML - if the image didn't previously exist, it is automatically created. Again, depends on what we're trying to achieve with the test driver, here I'd expect the app devs to follow a sequence of API calls, so they would want to call Save first, so that SaveImageDefineXML can be used subsequently. So ^this is meant to be kind of a discussion starter, code-wise, I'm also okay with what you have here, so: Reviewed-by: Erik Skultety <eskultet@redhat.com>
+ if ((def = virDomainDefParseString(dxml, privconn->caps, privconn->xmlopt, NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE)) == NULL) + goto cleanup; + + if (!testDomainSaveImageWrite(privconn, def, path)) + goto cleanup; + + ret = 0; + + cleanup: + virDomainDefFree(def); + return ret; +} + static int testDomainCoreDumpWithFormat(virDomainPtr domain, const char *to, unsigned int dumpformat, @@ -7010,6 +7037,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainSaveFlags = testDomainSaveFlags, /* 0.9.4 */ .domainRestore = testDomainRestore, /* 0.3.2 */ .domainRestoreFlags = testDomainRestoreFlags, /* 0.9.4 */ + .domainSaveImageDefineXML = testDomainSaveImageDefineXML, /* 5.4.0 */
This would be 5.5.0 Erik
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b0195ac63d..719e956d99 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2255,6 +2255,29 @@ testDomainSaveImageDefineXML(virConnectPtr conn, return ret; } +static char * +testDomainSaveImageGetXMLDesc(virConnectPtr conn, + const char *path, + unsigned int flags) +{ + int fd = -1; + char *ret = NULL; + virDomainDefPtr def = NULL; + testDriverPtr privconn = conn->privateData; + + virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); + + if ((fd = testDomainSaveImageOpen(privconn, path, &def)) < 0) + goto cleanup; + + ret = virDomainDefFormat(def, privconn->caps, VIR_DOMAIN_DEF_FORMAT_SECURE); + + cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + return ret; +} + static int testDomainCoreDumpWithFormat(virDomainPtr domain, const char *to, unsigned int dumpformat, @@ -7038,6 +7061,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainRestore = testDomainRestore, /* 0.3.2 */ .domainRestoreFlags = testDomainRestoreFlags, /* 0.9.4 */ .domainSaveImageDefineXML = testDomainSaveImageDefineXML, /* 5.4.0 */ + .domainSaveImageGetXMLDesc = testDomainSaveImageGetXMLDesc, /* 5.4.0 */ .domainCoreDump = testDomainCoreDump, /* 0.3.2 */ .domainCoreDumpWithFormat = testDomainCoreDumpWithFormat, /* 1.2.3 */ .domainSetVcpus = testDomainSetVcpus, /* 0.1.4 */ -- 2.21.0
On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
While implementing virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML for the test driver, I realized that there exists already code for saving and loading test images which can be reused. However, it needed to be extracted from testDomainSaveFlags and testDomainRestoreFlags into separate functions. The new functions are inspired by the corresponding QEMU driver code where e.g. qemuDomainSaveImageOpen serves as a helper used by other functions.
This series of patches initially extracts the code mentioned above into separate functions and then provides the test driver with implementations for virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML which make use of the newly introduced functions.
Ilias Stamatis (4): test_driver: extract image saving code into a separate function test_driver: extract image loading code into a separate function test_driver: implement virDomainSaveImageDefineXML test_driver: implement virDomainSaveImageGetXMLDesc
IMHO each public API should be introduced in a separate series. Erik
On Mon, Jun 03, 2019 at 10:36:58AM +0200, Erik Skultety wrote:
On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
While implementing virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML for the test driver, I realized that there exists already code for saving and loading test images which can be reused. However, it needed to be extracted from testDomainSaveFlags and testDomainRestoreFlags into separate functions. The new functions are inspired by the corresponding QEMU driver code where e.g. qemuDomainSaveImageOpen serves as a helper used by other functions.
This series of patches initially extracts the code mentioned above into separate functions and then provides the test driver with implementations for virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML which make use of the newly introduced functions.
Ilias Stamatis (4): test_driver: extract image saving code into a separate function test_driver: extract image loading code into a separate function test_driver: implement virDomainSaveImageDefineXML test_driver: implement virDomainSaveImageGetXMLDesc
IMHO each public API should be introduced in a separate series.
Any reason for that? These two are related and simple enough so to me it actually make sense to introduce them together in one series. Pavel
On Mon, Jun 3, 2019 at 1:50 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Mon, Jun 03, 2019 at 10:36:58AM +0200, Erik Skultety wrote:
On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
While implementing virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML for the test driver, I realized that there exists already code for saving and loading test images which can be reused. However, it needed to be extracted from testDomainSaveFlags and testDomainRestoreFlags into separate functions. The new functions are inspired by the corresponding QEMU driver code where e.g. qemuDomainSaveImageOpen serves as a helper used by other functions.
This series of patches initially extracts the code mentioned above into separate functions and then provides the test driver with implementations for virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML which make use of the newly introduced functions.
Ilias Stamatis (4): test_driver: extract image saving code into a separate function test_driver: extract image loading code into a separate function test_driver: implement virDomainSaveImageDefineXML test_driver: implement virDomainSaveImageGetXMLDesc
IMHO each public API should be introduced in a separate series.
Any reason for that? These two are related and simple enough so to me it actually make sense to introduce them together in one series.
Pavel
I also thought the same, since the patches touch certain parts of the test driver code and logic so it would make sense to be reviewed together by the same person. Ilias
On Mon, Jun 03, 2019 at 08:27:29PM +0200, Ilias Stamatis wrote:
On Mon, Jun 3, 2019 at 1:50 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Mon, Jun 03, 2019 at 10:36:58AM +0200, Erik Skultety wrote:
On Wed, May 29, 2019 at 02:22:55PM +0200, Ilias Stamatis wrote:
While implementing virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML for the test driver, I realized that there exists already code for saving and loading test images which can be reused. However, it needed to be extracted from testDomainSaveFlags and testDomainRestoreFlags into separate functions. The new functions are inspired by the corresponding QEMU driver code where e.g. qemuDomainSaveImageOpen serves as a helper used by other functions.
This series of patches initially extracts the code mentioned above into separate functions and then provides the test driver with implementations for virDomainSaveImageGetXMLDesc and virDomainSaveImageDefineXML which make use of the newly introduced functions.
Ilias Stamatis (4): test_driver: extract image saving code into a separate function test_driver: extract image loading code into a separate function test_driver: implement virDomainSaveImageDefineXML test_driver: implement virDomainSaveImageGetXMLDesc
IMHO each public API should be introduced in a separate series.
Any reason for that? These two are related and simple enough so to me it actually make sense to introduce them together in one series.
Pavel
I also thought the same, since the patches touch certain parts of the test driver code and logic so it would make sense to be reviewed together by the same person.
Ilias
That was just my personal taste, perhaps mostly internally driven by my comment in 2/4 which turned out to be a custom setting in my config. I don't have a strong preference though, it doesn't change anything on the review process, I would have continued (and I will) the review of this series anyway. Erik
participants (3)
-
Erik Skultety -
Ilias Stamatis -
Pavel Hrdina