Re: [PATCH 1/4] qemu-file: Don't do IO after shutdown

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Be sure that we are not doing neither read/write after shutdown of the
>> QEMUFile.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/qemu-file.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index 26fb25ddc1..1e5543a279 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -53,6 +53,8 @@ struct QEMUFile {
>>  
>>      int last_error;
>>      Error *last_error_obj;
>> +    /* has the file has been shutdown */
>> +    bool shutdown;
>>  };
>>  
>>  /*
>> @@ -61,6 +63,7 @@ struct QEMUFile {
>>   */
>>  int qemu_file_shutdown(QEMUFile *f)
>>  {
>> +    f->shutdown = true;
>>      if (!f->ops->shut_down) {
>>          return -ENOSYS;
>>      }
>> @@ -214,6 +217,9 @@ void qemu_fflush(QEMUFile *f)
>>          return;
>>      }
>>  
>> +    if (f->shutdown) {
>> +        return;
>> +    }
>
> OK, I did wonder if you need to free the iovec.

We need to improve things here.  We should free it on the 1st
error/shutdown.  Withought fixing all callers, I don't feel "safe" doing
it.

>
>>      if (f->iovcnt > 0) {
>>          expect = iov_size(f->iov, f->iovcnt);
>>          ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
>> @@ -328,6 +334,10 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
>>      f->buf_index = 0;
>>      f->buf_size = pending;
>>  
>> +    if (f->shutdown) {
>> +        return 0;
>> +    }
>
> I also wondered if perhaps an error would be reasonable here; but I'm
> not sure what a read(2) does after a shutdown(2).

We should check this sooner.  Same than prevoious.  If there has been an
error anywhere else, we should fail qemu_fill_buffer().  Right now we
don't do it. and we should.

qemu_get_error() and the setter should dissapear.  And we should just
return errors in all functions.  Especially now that we have migration
thread, and we don't have callbacks anymore.

> Still,
>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks, Juan.

>>      len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
>>                               IO_BUF_SIZE - pending, &local_error);
>>      if (len > 0) {
>> @@ -642,6 +652,9 @@ int64_t qemu_ftell(QEMUFile *f)
>>  
>>  int qemu_file_rate_limit(QEMUFile *f)
>>  {
>> +    if (f->shutdown) {
>> +        return 1;
>> +    }
>>      if (qemu_file_get_error(f)) {
>>          return 1;
>>      }
>> -- 
>> 2.23.0
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK