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(a)gmail.com&gt; --- 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 Mon, Jun 3, 2019 at 10:01 AM Erik Skultety <eskultet(a)redhat.com&gt; wrote: 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.

On Wed, May 29, 2019 at 02:22:58PM +0200, Ilias Stamatis wrote: 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(a)redhat.com&gt; This would be 5.5.0 Erik