Re: [PATCH V7 04/29] migration: simplify savevm

[Steven Sistare]

On 2/24/2022 1:25 PM, Dr. David Alan Gilbert wrote:
> * Steve Sistare (steven.sistare@oracle.com) wrote:
>> Use qemu_file_open to simplify a few functions in savevm.c.
>> No functional change.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> 
> So I think this is mostly OK, but a couple of minor tidyups below;
> so with the tidies and the renames from the previous patch:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Cool, thanks.

>> ---
>>  migration/savevm.c | 21 +++++++--------------
>>  1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 0bef031..c71d525 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2910,8 +2910,9 @@ bool save_snapshot(const char *name, bool overwrite, 
>> const char *vmstate,
>>  void qmp_xen_save_devices_state(const char *filename, bool has_live, bool 
>> live,
>>                                  Error **errp)
>>  {
>> +    const char *ioc_name = "migration-xen-save-state";
>> +    int flags = O_WRONLY | O_CREAT | O_TRUNC;
> 
> I don't see why to take these (or the matching ones in load) as separate
> variables; just keep them as is, and be parameters.

Will do.

- Steve

>>      QEMUFile *f;
>> -    QIOChannelFile *ioc;
>>      int saved_vm_running;
>>      int ret;
>>  
>> @@ -2925,14 +2926,10 @@ void qmp_xen_save_devices_state(const char 
>> *filename, bool has_live, bool live,
>>      vm_stop(RUN_STATE_SAVE_VM);
>>      global_state_store_running();
>>  
>> -    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT | O_TRUNC,
>> -                                    0660, errp);
>> -    if (!ioc) {
>> +    f = qemu_file_open(filename, flags, 0660, ioc_name, errp);
>> +    if (!f) {
>>          goto the_end;
>>      }
>> -    qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-save-state");
>> -    f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
>> -    object_unref(OBJECT(ioc));
>>      ret = qemu_save_device_state(f);
>>      if (ret < 0 || qemu_fclose(f) < 0) {
>>          error_setg(errp, QERR_IO_ERROR);
>> @@ -2960,8 +2957,8 @@ void qmp_xen_save_devices_state(const char *filename, 
>> bool has_live, bool live,
>>  
>>  void qmp_xen_load_devices_state(const char *filename, Error **errp)
>>  {
>> +    const char *ioc_name = "migration-xen-load-state";
>>      QEMUFile *f;
>> -    QIOChannelFile *ioc;
>>      int ret;
>>  
>>      /* Guest must be paused before loading the device state; the RAM state
>> @@ -2973,14 +2970,10 @@ void qmp_xen_load_devices_state(const char 
>> *filename, Error **errp)
>>      }
>>      vm_stop(RUN_STATE_RESTORE_VM);
>>  
>> -    ioc = qio_channel_file_new_path(filename, O_RDONLY | O_BINARY, 0, errp);
>> -    if (!ioc) {
>> +    f = qemu_file_open(filename, O_RDONLY | O_BINARY, 0, ioc_name, errp);
>> +    if (!f) {
>>          return;
>>      }
>> -    qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
>> -    f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
>> -    object_unref(OBJECT(ioc));
>> -
>>      ret = qemu_loadvm_state(f);
>>      qemu_fclose(f);
>>      if (ret < 0) {
>> -- 
>> 1.8.3.1
>>
>>