3:42 p.m.
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(a)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