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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]