Re: [libvirt] [PATCH] qemu_driver: pass path of compress prog directly

On 08/27/2016 03:13 AM, Chen Hanxiao wrote: > From: Chen Hanxiao <chenhanxiao(a)gmail.com&gt; > > 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(a)gmail.com&gt; > --- > 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.

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.

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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list