[libvirt] [PATCH v2 6/8] qemu: Use qemuGetCompressionProgram for error paths

[John Ferlan]

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 at 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