[libvirt] [PATCH v2 0/8] Pass the path of compress program (redo)
v1: http://www.redhat.com/archives/libvir-list/2016-September/msg00971.html Changes in v2: 1. Create a patch 5 which alters the qemuGetCompressionProgram to take a new const char * parameter which will be used in the warning message as the %s image format "style" (dump, save, snapshot) for both messages. Also removed the unnecessary translated message marker _() for the VIR_WARN message. 2. Alter former patch 5 to pass the style ("save" or "snapshot") as an argument for the altered virReportError messages which now will take a parameter that describe their style. For one message it keeps it constant, for the other it adds the style into the message. 3. Alter former patches 6 and 7 to account for the new argument (no real changes, just handling merge conflicts). (from patch 1's cover) 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 (8): qemu: Move getCompressionType qemu: Adjust doCoreDump to call getCompressionType qemu: Introduce helper qemuGetCompressionProgram qemu: Remove getCompressionType qemu: Alter qemuGetCompressionProgram warning message qemu: Use qemuGetCompressionProgram for error paths qemu: Remove qemuCompressProgramAvailable qemu: Get/return compressedpath program src/qemu/qemu_driver.c | 241 ++++++++++++++++++++++++------------------------- 1 file changed, 117 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 ee16cb5..af43695 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 af43695..956bddd 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 956bddd..3f03576 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
On Fri, Sep 23, 2016 at 07:30:51AM -0400, John Ferlan wrote:
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 956bddd..3f03576 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; +
The list of supported values is known at compile time. If the value is invalid, we should reject it when parsing the config file. That way the 'getCompressionProgram' helper would be faithful to its name and only try to get the compression program. Jan
On 09/27/2016 12:40 PM, Ján Tomko wrote:
On Fri, Sep 23, 2016 at 07:30:51AM -0400, John Ferlan wrote:
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 956bddd..3f03576 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; +
The list of supported values is known at compile time.
If the value is invalid, we should reject it when parsing the config file.
I'm sure we could if there was a patch for it, but for each of the 3 different types (dump, save, and snapshot) the "check" for validity was done within this scope (even before this set of patches). If one peeks at the virQEMUDriverConfigLoadFile where each of the cfg->{dump|save|snapshot}ImageFormat values are read, the validation of what's been read (for other fields) is minimal especially for strings. It seems it's left for "other" code to handle. I'll review a patch that validates the various fetches from qemu_conf.c and messages that "%s_image_format" has an invalid value.
That way the 'getCompressionProgram' helper would be faithful to its name and only try to get the compression program.
If the name is that much of an issue, then feel free to change the name. John
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 3f03576..d5acc30 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
Add a new parameter 'styleFormat' to be used when printing the warning message so that it's "clearer" what style of compression call caused the error. Add that style to both messages as a paremter. Also a VIR_WARN error message doesn't need to be translated (e.g. inside _()), so remove the need for the translation. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d5acc30..7dfba50 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3270,6 +3270,7 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) /* qemuGetCompressionProgram: * @imageFormat: String representation from qemu.conf for the compression * image format being used (dump, save, or snapshot). + * @styleFormat: String representing the style of format (dump, save, snapshot) * * Returns: * virQEMUSaveFormat - Integer representation of the compression @@ -3280,7 +3281,8 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) * indicating none. */ static virQEMUSaveFormat -qemuGetCompressionProgram(const char *imageFormat) +qemuGetCompressionProgram(const char *imageFormat, + const char *styleFormat) { virQEMUSaveFormat ret; @@ -3297,12 +3299,13 @@ qemuGetCompressionProgram(const char *imageFormat) error: if (ret < 0) - VIR_WARN("%s", _("Invalid dump image format specified in " - "configuration file, using raw")); + VIR_WARN("Invalid %s image format specified in " + "configuration file, using raw", + styleFormat); else - VIR_WARN("%s", _("Compression program for dump image format " - "in configuration file isn't available, " - "using raw")); + VIR_WARN("Compression program for %s image format in " + "configuration file isn't available, using raw", + styleFormat); /* Use "raw" as the format if the specified format is not valid, * or the compress program is not available. */ @@ -3590,7 +3593,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, "dump"); /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { -- 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 | 103 +++++++++++++++++++++---------------------------- 1 file changed, 43 insertions(+), 60 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7dfba50..8a47262 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3271,6 +3271,9 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) * @imageFormat: String representation from qemu.conf for the compression * image format being used (dump, save, or snapshot). * @styleFormat: String representing the style of format (dump, save, 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 @@ -3282,7 +3285,8 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) */ static virQEMUSaveFormat qemuGetCompressionProgram(const char *imageFormat, - const char *styleFormat) + const char *styleFormat, + bool use_raw_on_fail) { virQEMUSaveFormat ret; @@ -3298,18 +3302,34 @@ qemuGetCompressionProgram(const char *imageFormat, return ret; error: - if (ret < 0) - VIR_WARN("Invalid %s image format specified in " - "configuration file, using raw", - styleFormat); - else - VIR_WARN("Compression program for %s image format in " - "configuration file isn't available, using raw", - styleFormat); + if (ret < 0) { + if (use_raw_on_fail) + VIR_WARN("Invalid %s image format specified in " + "configuration file, using raw", + styleFormat); + else + virReportError(VIR_ERR_OPERATION_FAILED, + _("Invalid %s image format specified " + "in configuration file"), + styleFormat); + } else { + if (use_raw_on_fail) + VIR_WARN("Compression program for %s image format in " + "configuration file isn't available, using raw", + styleFormat); + else + virReportError(VIR_ERR_OPERATION_FAILED, + _("Compression program for %s image format " + "in configuration file isn't available"), + styleFormat); + } /* 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; } @@ -3318,7 +3338,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; @@ -3328,21 +3348,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, + "save", false)) < 0) + goto cleanup; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -3391,7 +3399,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; @@ -3418,21 +3426,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, + "save", false)) < 0) + goto cleanup; if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup; @@ -3593,7 +3589,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, "dump"); + compress = qemuGetCompressionProgram(cfg->dumpImageFormat, "dump", true); /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { @@ -14312,7 +14308,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. @@ -14373,22 +14369,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, + "snapshot", false)) < 0) + goto cleanup; if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true))) goto cleanup; -- 2.7.4
On Fri, Sep 23, 2016 at 07:30:54AM -0400, John Ferlan 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 | 103 +++++++++++++++++++++---------------------------- 1 file changed, 43 insertions(+), 60 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7dfba50..8a47262 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3271,6 +3271,9 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) * @imageFormat: String representation from qemu.conf for the compression * image format being used (dump, save, or snapshot). * @styleFormat: String representing the style of format (dump, save, 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 @@ -3282,7 +3285,8 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) */ static virQEMUSaveFormat qemuGetCompressionProgram(const char *imageFormat, - const char *styleFormat) + const char *styleFormat, + bool use_raw_on_fail) { virQEMUSaveFormat ret;
@@ -3298,18 +3302,34 @@ qemuGetCompressionProgram(const char *imageFormat, return ret;
error: - if (ret < 0) - VIR_WARN("Invalid %s image format specified in " - "configuration file, using raw", - styleFormat); - else - VIR_WARN("Compression program for %s image format in " - "configuration file isn't available, using raw", - styleFormat); + if (ret < 0) { + if (use_raw_on_fail) + VIR_WARN("Invalid %s image format specified in " + "configuration file, using raw", + styleFormat); + else + virReportError(VIR_ERR_OPERATION_FAILED, + _("Invalid %s image format specified " + "in configuration file"), + styleFormat); + } else { + if (use_raw_on_fail) + VIR_WARN("Compression program for %s image format in " + "configuration file isn't available, using raw", + styleFormat); + else + virReportError(VIR_ERR_OPERATION_FAILED, + _("Compression program for %s image format " + "in configuration file isn't available"), + styleFormat); + }
This might work for the English language, but constructing a translatable message in this matter is unfriendly to translators and not worth shaving off a few lines of code. Also, logging the "styleFormat" (sic) only makes sense for the dump, which might not have been a result of an API call, otherwise we're better off putting the image format in here. If you call the snapshot API and get an error saying "xz" was not found, you know it's not for the core dump format. Jan
On 09/27/2016 12:53 PM, Ján Tomko wrote:
On Fri, Sep 23, 2016 at 07:30:54AM -0400, John Ferlan 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 | 103 +++++++++++++++++++++---------------------------- 1 file changed, 43 insertions(+), 60 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7dfba50..8a47262 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3271,6 +3271,9 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) * @imageFormat: String representation from qemu.conf for the compression * image format being used (dump, save, or snapshot). * @styleFormat: String representing the style of format (dump, save, 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 @@ -3282,7 +3285,8 @@ qemuCompressProgramAvailable(virQEMUSaveFormat compress) */ static virQEMUSaveFormat qemuGetCompressionProgram(const char *imageFormat, - const char *styleFormat) + const char *styleFormat, + bool use_raw_on_fail) { virQEMUSaveFormat ret;
@@ -3298,18 +3302,34 @@ qemuGetCompressionProgram(const char *imageFormat, return ret;
error: - if (ret < 0) - VIR_WARN("Invalid %s image format specified in " - "configuration file, using raw", - styleFormat); - else - VIR_WARN("Compression program for %s image format in " - "configuration file isn't available, using raw", - styleFormat); + if (ret < 0) { + if (use_raw_on_fail) + VIR_WARN("Invalid %s image format specified in " + "configuration file, using raw", + styleFormat); + else + virReportError(VIR_ERR_OPERATION_FAILED, + _("Invalid %s image format specified " + "in configuration file"), + styleFormat); + } else { + if (use_raw_on_fail) + VIR_WARN("Compression program for %s image format in " + "configuration file isn't available, using raw", + styleFormat); + else + virReportError(VIR_ERR_OPERATION_FAILED, + _("Compression program for %s image format " + "in configuration file isn't available"), + styleFormat); + }
This might work for the English language, but constructing a translatable message in this matter is unfriendly to translators and not worth shaving off a few lines of code.
Hmm... And qemuDomainDiskChangeSupported is any better? Similarly qemuMonitorJSONGetBlockInfo? Instead of 3 (or 4) message pairs that we all similar varying only by "dump", "save", or "snapshot" there is now 1 message pair which has a %s parameter no different than 1000's of other libvirt messages. Since dump, save, and snapshot are all command names - does their translation really change? Beyond that I'll note that qemuNodeDeviceDetachFlags uses a const char *driverName which is just a string and can be messaged using %s. Also qemuConnectGetDomainCapabilities uses a const char *virttype_str which is messaged.
Also, logging the "styleFormat" (sic) only makes sense for the dump, which might not have been a result of an API call, otherwise we're better off putting the image format in here.
If you don't like the variable name, then change it. I'm not offended.
If you call the snapshot API and get an error saying "xz" was not found, you know it's not for the core dump format.
If you prefer a different mechanism, I'll review the patch when I see it. John
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 8a47262..6b755a5 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 @@ -3289,6 +3273,7 @@ qemuGetCompressionProgram(const char *imageFormat, bool use_raw_on_fail) { virQEMUSaveFormat ret; + char *path = NULL; if (!imageFormat) return QEMU_SAVE_FORMAT_RAW; @@ -3296,9 +3281,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 | 66 +++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6b755a5..fb2a8f6 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,10 +3248,13 @@ 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. * @styleFormat: String representing the style of format (dump, save, 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 + * and let the path remain as NULL. * * Returns: * virQEMUSaveFormat - Integer representation of the compression @@ -3267,13 +3264,15 @@ 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, const char *styleFormat, bool use_raw_on_fail) { virQEMUSaveFormat ret; - char *path = NULL; + + *compresspath = NULL; if (!imageFormat) return QEMU_SAVE_FORMAT_RAW; @@ -3281,11 +3280,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: @@ -3326,6 +3323,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; @@ -3336,6 +3334,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, + &compressedpath, "save", false)) < 0) goto cleanup; @@ -3352,10 +3351,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; } @@ -3387,6 +3387,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; @@ -3414,6 +3415,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, + &compressedpath, "save", false)) < 0) goto cleanup; @@ -3422,14 +3424,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; @@ -3572,11 +3575,15 @@ 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, "dump", 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, + "dump", true)); /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { @@ -3623,8 +3630,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); } @@ -3647,6 +3653,7 @@ doCoreDump(virQEMUDriverPtr driver, if (ret != 0) unlink(path); virFileWrapperFdFree(wrapperFd); + VIR_FREE(compressedpath); virObjectUnref(cfg); return ret; } @@ -14296,6 +14303,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. @@ -14357,15 +14365,16 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->snapshotImageFormat, + &compressedpath, "snapshot", 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 */ @@ -14434,6 +14443,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-23 19:30:48, "John Ferlan" <jferlan@redhat.com> wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-September/msg00971.html
Changes in v2:
1. Create a patch 5 which alters the qemuGetCompressionProgram to take a new const char * parameter which will be used in the warning message as the %s image format "style" (dump, save, snapshot) for both messages. Also removed the unnecessary translated message marker _() for the VIR_WARN message.
2. Alter former patch 5 to pass the style ("save" or "snapshot") as an argument for the altered virReportError messages which now will take a parameter that describe their style. For one message it keeps it constant, for the other it adds the style into the message.
3. Alter former patches 6 and 7 to account for the new argument (no real changes, just handling merge conflicts).
(from patch 1's cover)
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 (8): qemu: Move getCompressionType qemu: Adjust doCoreDump to call getCompressionType qemu: Introduce helper qemuGetCompressionProgram qemu: Remove getCompressionType qemu: Alter qemuGetCompressionProgram warning message qemu: Use qemuGetCompressionProgram for error paths qemu: Remove qemuCompressProgramAvailable qemu: Get/return compressedpath program
src/qemu/qemu_driver.c | 241 ++++++++++++++++++++++++------------------------- 1 file changed, 117 insertions(+), 124 deletions(-)
Looks good to me. ACK series. Regards, - Chen
participants (3)
-
Chen Hanxiao -
John Ferlan -
Ján Tomko