[libvirt] [PATCH 0/7] Pass the path of compress program (redo)

[John Ferlan]

On 09/21/2016 10:23 PM, Chen Hanxiao wrote:
> 
> At 2016-09-22 05:30:48, "John Ferlan" <jferlan at 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
>