At 2016-09-13 23:47:10, "John Ferlan" <jferlan@xxxxxxxxxx> wrote:
>
>
>On 08/27/2016 03:13 AM, Chen Hanxiao wrote:
>> From: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
>>
>> We check compress prog in qemuCompressProgramAvailable,
>> then check it again in virExec.
>>
>> This path will pass compress prog's path directly.
>>
>> Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
>> ---
>> src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++----------------
>> 1 file changed, 26 insertions(+), 16 deletions(-)
>>
>
>Perhaps a more detailed commit would explain what the purpose is of
>doing this. After a bit of thinking/research on this, essentially what
>you're doing is avoiding one extra virFindFileInPath call in during
>virExec when the passed program name isn't the complete path. Since
>callers of qemuDomainSaveMemory call qemuCompressProgramAvailable which
>does get that complete path - the desire is to use that result. I
>suppose that's an OK goal, but I think there could be more done.
>
>Even with this, qemuCompressProgramAvailable becomes more than just a
>bool function - it's' returning true/false *and* possibly a path to a
>file or NULL. The comment to the function is thus somewhat misleading
>because true can be returned w/ no program name if compress ==
>QEMU_SAVE_FORMAT_RAW.
>
>Finally, this patch only changes to provide the full path to
>qemuMigrationToFile from qemuDomainSaveMemory, but doCoreDump still
>passes just the program name.
>
>While looking through this is there's 3 callers that do pretty much the
>same sequence w/ the 'compressed' variable:
>
> cfg = virQEMUDriverGetConfig(driver)
> if (cfg->saveImageFormat) {
> ...
> qemuCompressProgramAvailable(...)
> }
>
>This sequence is very similar to how getCompressionType (called in each
>function calling doCoreDump) handles things except error processing is
>different - one uses VIR_WARN and returns QEMU_SAVE_FORMAT_RAW while the
>other uses virReportError and fails.
Maybe doCoreDump wants to continue even with a wrong compress config.
I think we shoud use a swith case for saveImageFormat, dumpImageFormat
and saveImageFormat.
>
>I'm thinking there's some "synergies" there that could help reduce code
>duplication and accomplish the "goal" that qemuMigrationToFile receives
>the path to the compression program or NULL when QEMU_SAVE_FORMAT_RAW.
>
>This would be a multiple patch type effort to essentially change things
>such that you receive a full path or NULL. Should be quite a few lines
>of code deleted. If you don't feel comfortable doing this, then let me
>know and I'll give it a shot.
I'll try to send a v2 in a few days.
Regards,
- Chen
>
>
>John
>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 97e2ffc..9f4e593 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -3037,6 +3037,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>> const char *path,
>> const char *domXML,
>> int compressed,
>> + const char *compressed_path,
>> bool was_running,
>> unsigned int flags,
>> qemuDomainAsyncJob asyncJob)
>> @@ -3084,7 +3085,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>> goto cleanup;
>>
>> /* Perform the migration */
>> - if (qemuMigrationToFile(driver, vm, fd, qemuCompressProgramName(compressed),
>> + if (qemuMigrationToFile(driver, vm, fd, compressed_path,
>> asyncJob) < 0)
>> goto cleanup;
>>
>> @@ -3137,7 +3138,8 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>> static int
>> qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
>> virDomainObjPtr vm, const char *path,
>> - int compressed, const char *xmlin, unsigned int flags)
>> + int compressed, const char *compressed_path,
>> + const char *xmlin, unsigned int flags)
>> {
>> char *xml = NULL;
>> bool was_running = false;
>> @@ -3209,7 +3211,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
>> goto endjob;
>> }
>>
>> - ret = qemuDomainSaveMemory(driver, vm, path, xml, compressed,
>> + ret = qemuDomainSaveMemory(driver, vm, path, xml, compressed, compressed_path,
>> was_running, flags, QEMU_ASYNC_JOB_SAVE);
>> if (ret < 0)
>> goto endjob;
>> @@ -3250,17 +3252,16 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
>>
>> /* Returns true if a compression program is available in PATH */
>> static bool
>> -qemuCompressProgramAvailable(virQEMUSaveFormat compress)
>> +qemuCompressProgramAvailable(virQEMUSaveFormat compress, char **compressed_path)
>> {
>> - char *path;
>> -
>> - if (compress == QEMU_SAVE_FORMAT_RAW)
>
>NIT: You could have set/initialized *compressed_path = NULL; here then
>there's no need for the setting in the if statement
>
>> + if (compress == QEMU_SAVE_FORMAT_RAW) {
>> + *compressed_path = NULL;
>> return true;
>> + }
>>
>> - if (!(path = virFindFileInPath(qemuSaveCompressionTypeToString(compress))))
>> + if (!(*compressed_path = virFindFileInPath(qemuSaveCompressionTypeToString(compress))))
>> return false;
>>
>> - VIR_FREE(path);
>> return true;
>> }
>>
>> @@ -3270,6 +3271,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,
>> {
>> virQEMUDriverPtr driver = dom->conn->privateData;
>> int compressed = QEMU_SAVE_FORMAT_RAW;
>> + char *compressed_path = NULL;
>> int ret = -1;
>> virDomainObjPtr vm = NULL;
>> virQEMUDriverConfigPtr cfg = NULL;
>> @@ -3287,7 +3289,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,
>> "in configuration file"));
>> goto cleanup;
>> }
>> - if (!qemuCompressProgramAvailable(compressed)) {
>> + if (!qemuCompressProgramAvailable(compressed, &compressed_path)) {
>> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> _("Compression program for image format "
>> "in configuration file isn't available"));
>> @@ -3308,11 +3310,12 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,
>> }
>>
>> ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed,
>> - dxml, flags);
>> + compressed_path, dxml, flags);
>>
>> cleanup:
>> virDomainObjEndAPI(&vm);
>> virObjectUnref(cfg);
>> + VIR_FREE(compressed_path);
>> return ret;
>> }
>>
>> @@ -3343,6 +3346,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
>> virQEMUDriverPtr driver = dom->conn->privateData;
>> virQEMUDriverConfigPtr cfg = NULL;
>> int compressed = QEMU_SAVE_FORMAT_RAW;
>> + char *compressed_path = NULL;
>> virDomainObjPtr vm;
>> char *name = NULL;
>> int ret = -1;
>> @@ -3377,7 +3381,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
>> "in configuration file"));
>> goto cleanup;
>> }
>> - if (!qemuCompressProgramAvailable(compressed)) {
>> + if (!qemuCompressProgramAvailable(compressed, &compressed_path)) {
>> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> _("Compression program for image format "
>> "in configuration file isn't available"));
>> @@ -3391,13 +3395,14 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
>> VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, name);
>>
>> ret = qemuDomainSaveInternal(driver, dom, vm, name,
>> - compressed, NULL, flags);
>> + compressed, compressed_path, NULL, flags);
>> if (ret == 0)
>> vm->hasManagedSave = true;
>>
>> cleanup:
>> virDomainObjEndAPI(&vm);
>> VIR_FREE(name);
>> + VIR_FREE(compressed_path);
>> virObjectUnref(cfg);
>>
>> return ret;
>> @@ -3617,6 +3622,7 @@ static virQEMUSaveFormat
>> getCompressionType(virQEMUDriverPtr driver)
>> {
>> int ret = QEMU_SAVE_FORMAT_RAW;
>> + char *compressed_path = NULL;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>
>> /*
>> @@ -3634,7 +3640,7 @@ getCompressionType(virQEMUDriverPtr driver)
>> ret = QEMU_SAVE_FORMAT_RAW;
>> goto cleanup;
>> }
>> - if (!qemuCompressProgramAvailable(ret)) {
>> + if (!qemuCompressProgramAvailable(ret, &compressed_path)) {
>> VIR_WARN("%s", _("Compression program for dump image format "
>> "in configuration file isn't available, "
>> "using raw"));
>> @@ -3644,6 +3650,7 @@ getCompressionType(virQEMUDriverPtr driver)
>> }
>> cleanup:
>> virObjectUnref(cfg);
>> + VIR_FREE(compressed_path);
>> return ret;
>> }
>>
>> @@ -14308,6 +14315,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>> bool pmsuspended = false;
>> virQEMUDriverConfigPtr cfg = NULL;
>> int compressed = QEMU_SAVE_FORMAT_RAW;
>> + char *compressed_path = NULL;
>>
>> /* If quiesce was requested, then issue a freeze command, and a
>> * counterpart thaw command when it is actually sent to agent.
>> @@ -14377,7 +14385,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>> goto cleanup;
>> }
>>
>> - if (!qemuCompressProgramAvailable(compressed)) {
>> + if (!qemuCompressProgramAvailable(compressed, &compressed_path)) {
>> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> _("Compression program for image format "
>> "in configuration file isn't available"));
>> @@ -14389,7 +14397,8 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>> goto cleanup;
>>
>> if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file,
>> - xml, compressed, resume, 0,
>> + xml, compressed, compressed_path,
>> + resume, 0,
>> QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
>> goto cleanup;
>>
>> @@ -14459,6 +14468,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>> }
>>
>> VIR_FREE(xml);
>> + VIR_FREE(compressed_path);
>> virObjectUnref(cfg);
>> if (memory_unlink && ret < 0)
>> unlink(snap->def->file);
>>
>
>--
>libvir-list mailing list
>libvir-list@xxxxxxxxxx
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list