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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list