[libvirt] [PATCH 0/7] Pass the path of compress program (redo)
Rather than try to describe what I was thinking about for the passing the compress program directly series from Chen Hanxiao: http://www.redhat.com/archives/libvir-list/2016-September/msg00677.html and http://www.redhat.com/archives/libvir-list/2016-August/msg01237.html I took the liberty of creating my own set of patches which should end in the more or less same result. The primary benefit is not calling virFileInPath twice since we already get it at least once for the various compressed program callers, let's pass what we originally found. John Ferlan (7): qemu: Move getCompressionType qemu: Adjust doCoreDump to call getCompressionType qemu: Introduce helper qemuGetCompressionProgram qemu: Remove getCompressionType qemu: Use qemuGetCompressionProgram for error paths qemu: Remove qemuCompressProgramAvailable qemu: Get/return compressedpath program src/qemu/qemu_driver.c | 235 +++++++++++++++++++++++-------------------------- 1 file changed, 111 insertions(+), 124 deletions(-) -- 2.7.4
A subsequent patch will adjust the 3 callers to just call from doCoreDump. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 70 ++++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 71eb062..9b6ac48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3266,6 +3266,42 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) return true; } + +static virQEMUSaveFormat +getCompressionType(virQEMUDriverPtr driver) +{ + int ret = QEMU_SAVE_FORMAT_RAW; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + /* + * We reuse "save" flag for "dump" here. Then, we can support the same + * format in "save" and "dump". + */ + if (cfg->dumpImageFormat) { + ret = qemuSaveCompressionTypeFromString(cfg->dumpImageFormat); + /* Use "raw" as the format if the specified format is not valid, + * or the compress program is not available. + */ + if (ret < 0) { + VIR_WARN("%s", _("Invalid dump image format specified in " + "configuration file, using raw")); + ret = QEMU_SAVE_FORMAT_RAW; + goto cleanup; + } + if (!qemuCompressProgramAvailable(ret)) { + VIR_WARN("%s", _("Compression program for dump image format " + "in configuration file isn't available, " + "using raw")); + ret = QEMU_SAVE_FORMAT_RAW; + goto cleanup; + } + } + cleanup: + virObjectUnref(cfg); + return ret; +} + + static int qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, unsigned int flags) @@ -3615,40 +3651,6 @@ doCoreDump(virQEMUDriverPtr driver, return ret; } -static virQEMUSaveFormat -getCompressionType(virQEMUDriverPtr driver) -{ - int ret = QEMU_SAVE_FORMAT_RAW; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - - /* - * We reuse "save" flag for "dump" here. Then, we can support the same - * format in "save" and "dump". - */ - if (cfg->dumpImageFormat) { - ret = qemuSaveCompressionTypeFromString(cfg->dumpImageFormat); - /* Use "raw" as the format if the specified format is not valid, - * or the compress program is not available. - */ - if (ret < 0) { - VIR_WARN("%s", _("Invalid dump image format specified in " - "configuration file, using raw")); - ret = QEMU_SAVE_FORMAT_RAW; - goto cleanup; - } - if (!qemuCompressProgramAvailable(ret)) { - VIR_WARN("%s", _("Compression program for dump image format " - "in configuration file isn't available, " - "using raw")); - ret = QEMU_SAVE_FORMAT_RAW; - goto cleanup; - } - } - cleanup: - virObjectUnref(cfg); - return ret; -} - static int qemuDomainCoreDumpWithFormat(virDomainPtr dom, -- 2.7.4
Rather than calling getCompressionType from each of the callers, just call it from doCoreDump. A subsequent patch will be adjust the code even more. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9b6ac48..6dfdaa8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3568,7 +3568,6 @@ static int doCoreDump(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *path, - virQEMUSaveFormat compress, unsigned int dump_flags, unsigned int dumpformat) { @@ -3578,6 +3577,7 @@ doCoreDump(virQEMUDriverPtr driver, int directFlag = 0; unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING; const char *memory_dump_format = NULL; + virQEMUSaveFormat compress = getCompressionType(driver); /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { @@ -3704,9 +3704,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, } } - ret = doCoreDump(driver, vm, path, getCompressionType(driver), flags, - dumpformat); - if (ret < 0) + if ((ret = doCoreDump(driver, vm, path, flags, dumpformat)) < 0) goto endjob; paused = true; @@ -3911,10 +3909,8 @@ processWatchdogEvent(virQEMUDriverPtr driver, } flags |= cfg->autoDumpBypassCache ? VIR_DUMP_BYPASS_CACHE: 0; - ret = doCoreDump(driver, vm, dumpfile, - getCompressionType(driver), flags, - VIR_DOMAIN_CORE_DUMP_FORMAT_RAW); - if (ret < 0) + if ((ret = doCoreDump(driver, vm, dumpfile, flags, + VIR_DOMAIN_CORE_DUMP_FORMAT_RAW)) < 0) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Dump failed")); @@ -3951,10 +3947,8 @@ doCoreDumpToAutoDumpPath(virQEMUDriverPtr driver, goto cleanup; flags |= cfg->autoDumpBypassCache ? VIR_DUMP_BYPASS_CACHE: 0; - ret = doCoreDump(driver, vm, dumpfile, - getCompressionType(driver), flags, - VIR_DOMAIN_CORE_DUMP_FORMAT_RAW); - if (ret < 0) + if ((ret = doCoreDump(driver, vm, dumpfile, flags, + VIR_DOMAIN_CORE_DUMP_FORMAT_RAW)) < 0) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Dump failed")); cleanup: -- 2.7.4
Split out the guts of getCompressionType to perform the same functionality in the new helper program with a subsequent patch goal to be reusable for other callers making similar checks/calls to ensure the compression type is valid and that the compression program cannot be found. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6dfdaa8..1c41c7d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3267,36 +3267,61 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) } +/* qemuGetCompressionProgram: + * @imageFormat: String representation from qemu.conf for the compression + * image format being used (dump, save, or snapshot). + * + * Returns: + * virQEMUSaveFormat - Integer representation of the compression + * program to be used for particular style + * (e.g. dump, save, or snapshot). + * QEMU_SAVE_FORMAT_RAW - If there is no qemu.conf imageFormat value or + * no there was an error, then just return RAW + * indicating none. + */ +static virQEMUSaveFormat +qemuGetCompressionProgram(const char *imageFormat) +{ + virQEMUSaveFormat ret; + + if (!imageFormat) + return QEMU_SAVE_FORMAT_RAW; + + if ((ret = qemuSaveCompressionTypeFromString(imageFormat)) < 0) + goto error; + + if (!qemuCompressProgramAvailable(ret)) + goto error; + + return ret; + + error: + if (ret < 0) + VIR_WARN("%s", _("Invalid dump image format specified in " + "configuration file, using raw")); + else + VIR_WARN("%s", _("Compression program for dump image format " + "in configuration file isn't available, " + "using raw")); + + /* Use "raw" as the format if the specified format is not valid, + * or the compress program is not available. */ + return QEMU_SAVE_FORMAT_RAW; +} + + static virQEMUSaveFormat getCompressionType(virQEMUDriverPtr driver) { - int ret = QEMU_SAVE_FORMAT_RAW; + int ret; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); /* * We reuse "save" flag for "dump" here. Then, we can support the same * format in "save" and "dump". */ - if (cfg->dumpImageFormat) { - ret = qemuSaveCompressionTypeFromString(cfg->dumpImageFormat); - /* Use "raw" as the format if the specified format is not valid, - * or the compress program is not available. - */ - if (ret < 0) { - VIR_WARN("%s", _("Invalid dump image format specified in " - "configuration file, using raw")); - ret = QEMU_SAVE_FORMAT_RAW; - goto cleanup; - } - if (!qemuCompressProgramAvailable(ret)) { - VIR_WARN("%s", _("Compression program for dump image format " - "in configuration file isn't available, " - "using raw")); - ret = QEMU_SAVE_FORMAT_RAW; - goto cleanup; - } - } - cleanup: + ret = qemuGetCompressionProgram(cfg->dumpImageFormat); + virObjectUnref(cfg); return ret; } -- 2.7.4
There's only one caller now anyway... Besides it's just a shell for getting the compress type. Subsequent patches will return the path to the compression program. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1c41c7d..4cd0a07 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3310,23 +3310,6 @@ qemuGetCompressionProgram(const char *imageFormat) } -static virQEMUSaveFormat -getCompressionType(virQEMUDriverPtr driver) -{ - int ret; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - - /* - * We reuse "save" flag for "dump" here. Then, we can support the same - * format in "save" and "dump". - */ - ret = qemuGetCompressionProgram(cfg->dumpImageFormat); - - virObjectUnref(cfg); - return ret; -} - - static int qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, unsigned int flags) @@ -3602,7 +3585,12 @@ doCoreDump(virQEMUDriverPtr driver, int directFlag = 0; unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING; const char *memory_dump_format = NULL; - virQEMUSaveFormat compress = getCompressionType(driver); + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virQEMUSaveFormat compress; + + /* We reuse "save" flag for "dump" here. Then, we can support the same + * format in "save" and "dump". */ + compress = qemuGetCompressionProgram(cfg->dumpImageFormat); /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { @@ -3673,6 +3661,7 @@ doCoreDump(virQEMUDriverPtr driver, if (ret != 0) unlink(path); virFileWrapperFdFree(wrapperFd); + virObjectUnref(cfg); return ret; } -- 2.7.4
Let's do some more code reuse - there are 3 other callers that care to check/get the compress program. Each of those though cares whether the requested cfg image is valid and exists. So, add a parameter to handle those cases. NB: We won't need to initialize the returned value in the case where the cfg image doesn't exist since the called program will handle that. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 99 ++++++++++++++++++++------------------------------ 1 file changed, 40 insertions(+), 59 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4cd0a07..505dd29 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3270,6 +3270,9 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) /* qemuGetCompressionProgram: * @imageFormat: String representation from qemu.conf for the compression * image format being used (dump, save, or snapshot). + * @use_raw_on_fail: Boolean indicating how to handle the error path. For + * callers that are OK with invalid data or inability to + * find the compression program, just return a raw format * * Returns: * virQEMUSaveFormat - Integer representation of the compression @@ -3280,7 +3283,8 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) * indicating none. */ static virQEMUSaveFormat -qemuGetCompressionProgram(const char *imageFormat) +qemuGetCompressionProgram(const char *imageFormat, + bool use_raw_on_fail) { virQEMUSaveFormat ret; @@ -3296,17 +3300,31 @@ qemuGetCompressionProgram(const char *imageFormat) return ret; error: - if (ret < 0) - VIR_WARN("%s", _("Invalid dump image format specified in " - "configuration file, using raw")); - else - VIR_WARN("%s", _("Compression program for dump image format " - "in configuration file isn't available, " - "using raw")); + if (ret < 0) { + if (use_raw_on_fail) + VIR_WARN("%s", _("Invalid dump image format specified in " + "configuration file, using raw")); + else + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Invalid save image format specified " + "in configuration file")); + } else { + if (use_raw_on_fail) + VIR_WARN("%s", _("Compression program for dump image format " + "in configuration file isn't available, " + "using raw")); + else + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Compression program for image format " + "in configuration file isn't available")); + } /* Use "raw" as the format if the specified format is not valid, * or the compress program is not available. */ - return QEMU_SAVE_FORMAT_RAW; + if (use_raw_on_fail) + return QEMU_SAVE_FORMAT_RAW; + + return -1; } @@ -3315,7 +3333,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - int compressed = QEMU_SAVE_FORMAT_RAW; + int compressed; int ret = -1; virDomainObjPtr vm = NULL; virQEMUDriverConfigPtr cfg = NULL; @@ -3325,21 +3343,9 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, VIR_DOMAIN_SAVE_PAUSED, -1); cfg = virQEMUDriverGetConfig(driver); - if (cfg->saveImageFormat) { - compressed = qemuSaveCompressionTypeFromString(cfg->saveImageFormat); - if (compressed < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid save image format specified " - "in configuration file")); - goto cleanup; - } - if (!qemuCompressProgramAvailable(compressed)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Compression program for image format " - "in configuration file isn't available")); - goto cleanup; - } - } + if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, + false)) < 0) + goto cleanup; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -3388,7 +3394,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; virQEMUDriverConfigPtr cfg = NULL; - int compressed = QEMU_SAVE_FORMAT_RAW; + int compressed; virDomainObjPtr vm; char *name = NULL; int ret = -1; @@ -3415,21 +3421,9 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) } cfg = virQEMUDriverGetConfig(driver); - if (cfg->saveImageFormat) { - compressed = qemuSaveCompressionTypeFromString(cfg->saveImageFormat); - if (compressed < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid save image format specified " - "in configuration file")); - goto cleanup; - } - if (!qemuCompressProgramAvailable(compressed)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Compression program for image format " - "in configuration file isn't available")); - goto cleanup; - } - } + if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, + false)) < 0) + goto cleanup; if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup; @@ -3590,7 +3584,7 @@ doCoreDump(virQEMUDriverPtr driver, /* We reuse "save" flag for "dump" here. Then, we can support the same * format in "save" and "dump". */ - compress = qemuGetCompressionProgram(cfg->dumpImageFormat); + compress = qemuGetCompressionProgram(cfg->dumpImageFormat, true); /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { @@ -14318,7 +14312,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool pmsuspended = false; virQEMUDriverConfigPtr cfg = NULL; - int compressed = QEMU_SAVE_FORMAT_RAW; + int compressed; /* If quiesce was requested, then issue a freeze command, and a * counterpart thaw command when it is actually sent to agent. @@ -14379,22 +14373,9 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, JOB_MASK(QEMU_JOB_MIGRATION_OP))); cfg = virQEMUDriverGetConfig(driver); - if (cfg->snapshotImageFormat) { - compressed = qemuSaveCompressionTypeFromString(cfg->snapshotImageFormat); - if (compressed < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid snapshot image format specified " - "in configuration file")); - goto cleanup; - } - - if (!qemuCompressProgramAvailable(compressed)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Compression program for image format " - "in configuration file isn't available")); - goto cleanup; - } - } + if ((compressed = qemuGetCompressionProgram(cfg->snapshotImageFormat, + false)) < 0) + goto cleanup; if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true))) goto cleanup; -- 2.7.4
At 2016-09-22 05:30:53, "John Ferlan" <jferlan@redhat.com> wrote:
Let's do some more code reuse - there are 3 other callers that care to check/get the compress program. Each of those though cares whether the requested cfg image is valid and exists. So, add a parameter to handle those cases.
NB: We won't need to initialize the returned value in the case where the cfg image doesn't exist since the called program will handle that.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 99 ++++++++++++++++++++------------------------------ 1 file changed, 40 insertions(+), 59 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4cd0a07..505dd29 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3270,6 +3270,9 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) /* qemuGetCompressionProgram: * @imageFormat: String representation from qemu.conf for the compression * image format being used (dump, save, or snapshot). + * @use_raw_on_fail: Boolean indicating how to handle the error path. For + * callers that are OK with invalid data or inability to + * find the compression program, just return a raw format * * Returns: * virQEMUSaveFormat - Integer representation of the compression @@ -3280,7 +3283,8 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) * indicating none. */ static virQEMUSaveFormat -qemuGetCompressionProgram(const char *imageFormat) +qemuGetCompressionProgram(const char *imageFormat, + bool use_raw_on_fail) { virQEMUSaveFormat ret;
@@ -3296,17 +3300,31 @@ qemuGetCompressionProgram(const char *imageFormat) return ret;
error: - if (ret < 0) - VIR_WARN("%s", _("Invalid dump image format specified in " - "configuration file, using raw")); - else - VIR_WARN("%s", _("Compression program for dump image format " - "in configuration file isn't available, " - "using raw")); + if (ret < 0) { + if (use_raw_on_fail) + VIR_WARN("%s", _("Invalid dump image format specified in " + "configuration file, using raw")); + else + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Invalid save image format specified " + "in configuration file")); + } else { + if (use_raw_on_fail) + VIR_WARN("%s", _("Compression program for dump image format " + "in configuration file isn't available, " + "using raw")); + else + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Compression program for image format " + "in configuration file isn't available")); + }
Actually we had to deal with: saveImageFormat snapshotImageFormat dumpImageFormat I think a switch case with a macro should be fitted. Patch 6, 7 looks good to me. Regards, - Chen
There's only one caller and the code is duplicitous just converting the recently converted cfg image name back into it's string value in order to get/find the path to the image. A subsequent patch can return this path. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 505dd29..18fd87d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3250,22 +3250,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, return ret; } -/* Returns true if a compression program is available in PATH */ -static bool -qemuCompressProgramAvailable(virQEMUSaveFormat compress) -{ - char *path; - - if (compress == QEMU_SAVE_FORMAT_RAW) - return true; - - if (!(path = virFindFileInPath(qemuSaveCompressionTypeToString(compress)))) - return false; - - VIR_FREE(path); - return true; -} - /* qemuGetCompressionProgram: * @imageFormat: String representation from qemu.conf for the compression @@ -3287,6 +3271,7 @@ qemuGetCompressionProgram(const char *imageFormat, bool use_raw_on_fail) { virQEMUSaveFormat ret; + char *path = NULL; if (!imageFormat) return QEMU_SAVE_FORMAT_RAW; @@ -3294,9 +3279,11 @@ qemuGetCompressionProgram(const char *imageFormat, if ((ret = qemuSaveCompressionTypeFromString(imageFormat)) < 0) goto error; - if (!qemuCompressProgramAvailable(ret)) + if (!(path = virFindFileInPath(imageFormat))) goto error; + VIR_FREE(path); + return ret; error: -- 2.7.4
Based upon a patch from Chen Hanxiao <chenhanxiao@gmail.com>, rather than need to call virFindFileInPath twice, let's just save the path and pass it along with the compressed type. (NB: the second call would be in virExec as called from virCommandRunAsync which is called from qemuMigrationToFile using the argument 'compressor' which up to this point would be the string from the cfg file that isn't the fully qualified path). Since we now have the path, we can remove qemuCompressProgramName which would return NULL or the string representation of the compress type. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 65 ++++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 18fd87d..abc34fc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2833,14 +2833,6 @@ qemuDomainSaveHeader(int fd, const char *path, const char *xml, return ret; } -/* Given a virQEMUSaveFormat compression level, return the name - * of the program to run, or NULL if no program is needed. */ -static const char * -qemuCompressProgramName(int compress) -{ - return (compress == QEMU_SAVE_FORMAT_RAW ? NULL : - qemuSaveCompressionTypeToString(compress)); -} static virCommandPtr qemuCompressGetCommand(virQEMUSaveFormat compression) @@ -3039,6 +3031,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, const char *path, const char *domXML, int compressed, + const char *compressedpath, bool was_running, unsigned int flags, qemuDomainAsyncJob asyncJob) @@ -3086,8 +3079,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, goto cleanup; /* Perform the migration */ - if (qemuMigrationToFile(driver, vm, fd, qemuCompressProgramName(compressed), - asyncJob) < 0) + if (qemuMigrationToFile(driver, vm, fd, compressedpath, asyncJob) < 0) goto cleanup; /* Touch up file header to mark image complete. */ @@ -3139,7 +3131,8 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, static int qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, virDomainObjPtr vm, const char *path, - int compressed, const char *xmlin, unsigned int flags) + int compressed, const char *compressedpath, + const char *xmlin, unsigned int flags) { char *xml = NULL; bool was_running = false; @@ -3212,7 +3205,8 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, } ret = qemuDomainSaveMemory(driver, vm, path, xml, compressed, - was_running, flags, QEMU_ASYNC_JOB_SAVE); + compressedpath, was_running, flags, + QEMU_ASYNC_JOB_SAVE); if (ret < 0) goto endjob; @@ -3254,9 +3248,12 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, /* qemuGetCompressionProgram: * @imageFormat: String representation from qemu.conf for the compression * image format being used (dump, save, or snapshot). + * @compresspath: Pointer to a character string to store the fully qualified + * path from virFindFileInPath. * @use_raw_on_fail: Boolean indicating how to handle the error path. For * callers that are OK with invalid data or inability to * find the compression program, just return a raw format + * and let the path remain as NULL. * * Returns: * virQEMUSaveFormat - Integer representation of the compression @@ -3266,12 +3263,14 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, * no there was an error, then just return RAW * indicating none. */ -static virQEMUSaveFormat +static virQEMUSaveFormat ATTRIBUTE_NONNULL(2) qemuGetCompressionProgram(const char *imageFormat, + char **compresspath, bool use_raw_on_fail) { virQEMUSaveFormat ret; - char *path = NULL; + + *compresspath = NULL; if (!imageFormat) return QEMU_SAVE_FORMAT_RAW; @@ -3279,11 +3278,9 @@ qemuGetCompressionProgram(const char *imageFormat, if ((ret = qemuSaveCompressionTypeFromString(imageFormat)) < 0) goto error; - if (!(path = virFindFileInPath(imageFormat))) + if (!(*compresspath = virFindFileInPath(imageFormat))) goto error; - VIR_FREE(path); - return ret; error: @@ -3321,6 +3318,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, { virQEMUDriverPtr driver = dom->conn->privateData; int compressed; + char *compressedpath = NULL; int ret = -1; virDomainObjPtr vm = NULL; virQEMUDriverConfigPtr cfg = NULL; @@ -3331,6 +3329,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, + &compressedpath, false)) < 0) goto cleanup; @@ -3347,10 +3346,11 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, } ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed, - dxml, flags); + compressedpath, dxml, flags); cleanup: virDomainObjEndAPI(&vm); + VIR_FREE(compressedpath); virObjectUnref(cfg); return ret; } @@ -3382,6 +3382,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) virQEMUDriverPtr driver = dom->conn->privateData; virQEMUDriverConfigPtr cfg = NULL; int compressed; + char *compressedpath = NULL; virDomainObjPtr vm; char *name = NULL; int ret = -1; @@ -3409,6 +3410,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, + &compressedpath, false)) < 0) goto cleanup; @@ -3417,14 +3419,15 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, name); - ret = qemuDomainSaveInternal(driver, dom, vm, name, - compressed, NULL, flags); + ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, + compressedpath, NULL, flags); if (ret == 0) vm->hasManagedSave = true; cleanup: virDomainObjEndAPI(&vm); VIR_FREE(name); + VIR_FREE(compressedpath); virObjectUnref(cfg); return ret; @@ -3567,11 +3570,14 @@ doCoreDump(virQEMUDriverPtr driver, unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING; const char *memory_dump_format = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virQEMUSaveFormat compress; + char *compressedpath = NULL; /* We reuse "save" flag for "dump" here. Then, we can support the same - * format in "save" and "dump". */ - compress = qemuGetCompressionProgram(cfg->dumpImageFormat, true); + * format in "save" and "dump". This path doesn't need the compression + * program to exist and can ignore the return value - it only cares to + * get the compressedpath */ + ignore_value(qemuGetCompressionProgram(cfg->dumpImageFormat, + &compressedpath, true)); /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { @@ -3618,8 +3624,7 @@ doCoreDump(virQEMUDriverPtr driver, if (!qemuMigrationIsAllowed(driver, vm, false, 0)) goto cleanup; - ret = qemuMigrationToFile(driver, vm, fd, - qemuCompressProgramName(compress), + ret = qemuMigrationToFile(driver, vm, fd, compressedpath, QEMU_ASYNC_JOB_DUMP); } @@ -3642,6 +3647,7 @@ doCoreDump(virQEMUDriverPtr driver, if (ret != 0) unlink(path); virFileWrapperFdFree(wrapperFd); + VIR_FREE(compressedpath); virObjectUnref(cfg); return ret; } @@ -14300,6 +14306,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, bool pmsuspended = false; virQEMUDriverConfigPtr cfg = NULL; int compressed; + char *compressedpath = NULL; /* If quiesce was requested, then issue a freeze command, and a * counterpart thaw command when it is actually sent to agent. @@ -14361,15 +14368,16 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->snapshotImageFormat, + &compressedpath, false)) < 0) goto cleanup; if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true))) goto cleanup; - if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file, - xml, compressed, resume, 0, - QEMU_ASYNC_JOB_SNAPSHOT)) < 0) + if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file, xml, + compressed, compressedpath, resume, + 0, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; /* the memory image was created, remove it on errors */ @@ -14438,6 +14446,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, } VIR_FREE(xml); + VIR_FREE(compressedpath); virObjectUnref(cfg); if (memory_unlink && ret < 0) unlink(snap->def->file); -- 2.7.4
At 2016-09-22 05:30:48, "John Ferlan" <jferlan@redhat.com> wrote:
Rather than try to describe what I was thinking about for the passing the compress program directly series from Chen Hanxiao:
http://www.redhat.com/archives/libvir-list/2016-September/msg00677.html
and
http://www.redhat.com/archives/libvir-list/2016-August/msg01237.html
I took the liberty of creating my own set of patches which should end in the more or less same result.
The primary benefit is not calling virFileInPath twice since we already get it at least once for the various compressed program callers, let's pass what we originally found.
John Ferlan (7): qemu: Move getCompressionType qemu: Adjust doCoreDump to call getCompressionType qemu: Introduce helper qemuGetCompressionProgram qemu: Remove getCompressionType qemu: Use qemuGetCompressionProgram for error paths qemu: Remove qemuCompressProgramAvailable qemu: Get/return compressedpath program
src/qemu/qemu_driver.c | 235 +++++++++++++++++++++++-------------------------- 1 file changed, 111 insertions(+), 124 deletions(-)
--
Hi, John Looks that Patch 1, 3 and 4 should be scrash into one :). Some other inline comments, Please check. Regards, - Chen
On 09/21/2016 10:23 PM, Chen Hanxiao wrote:
At 2016-09-22 05:30:48, "John Ferlan" <jferlan@redhat.com> wrote:
Rather than try to describe what I was thinking about for the passing the compress program directly series from Chen Hanxiao:
http://www.redhat.com/archives/libvir-list/2016-September/msg00677.html
and
http://www.redhat.com/archives/libvir-list/2016-August/msg01237.html
I took the liberty of creating my own set of patches which should end in the more or less same result.
The primary benefit is not calling virFileInPath twice since we already get it at least once for the various compressed program callers, let's pass what we originally found.
John Ferlan (7): qemu: Move getCompressionType qemu: Adjust doCoreDump to call getCompressionType qemu: Introduce helper qemuGetCompressionProgram qemu: Remove getCompressionType qemu: Use qemuGetCompressionProgram for error paths qemu: Remove qemuCompressProgramAvailable qemu: Get/return compressedpath program
src/qemu/qemu_driver.c | 235 +++++++++++++++++++++++-------------------------- 1 file changed, 111 insertions(+), 124 deletions(-)
--
Hi, John
Looks that Patch 1, 3 and 4 should be scrash into one :).
I disagree - then in becomes one mish-mash of a patch which is harder to review. The way I split things up (for me) was a logical progression of this series. 1. Move code because we're about to use it from doCoreDump. 2. Use getCompressionType from within doCoreDump instead of passing it 3. Split out the "cfg->dumpImageFormat" into a separate helper and call 4. Remove need for getCompressionType for doCoreDump As for the rest 5. Create new parameter to change how messaging is done. From doCoreDump we can VIR_WARN since the code can continue; whereas, from qemuDomainSaveFlags, qemuDomainManagedSave, and qemuDomainSnapshotCreateActiveExternal we want to have the error and fail. Using "ret < 0" in the error path allows us to know which of the two options failed. Yes, the error message for snapshot will now be different, but that's far more easily fixed by a new parameter than the extra code created in your patch 2 to handle that error message. 6. Remove need for qemuCompressProgramAvailable by calling virFindFileInPath directly. No need to worry about the QEMU_SAVE_FORMAT_RAW case since it's already handled. 7. Return that compressed program name instead of calling qemuCompressProgramName again. Again, since QEMU_SAVE_FORMAT_RAW was already handled we're good.
Some other inline comments, Please check.
I'll post an adjustment for patch 5 error message tomorrow - it won't be complicated. Tks for the comments/review - John
Regards, - Chen
participants (2)
-
Chen Hanxiao -
John Ferlan