On 09/21/2016 10:23 PM, Chen Hanxiao wrote: > > At 2016-09-22 05:30:48, "John Ferlan" <jferlan@xxxxxxxxxx> 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list