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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "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.
> 
> I will think about this one.
> 
> >>      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).
> 
> A fast google shows that it is .... implementation dependant.  And
> worse, only really works for sockets.

Yeh, so our main reason for using it is for hung sockets; in particular,
if a machine just disappears, then you get a many-minute hang waiting
for TCP to timeout.  Using 'shutdown(2)' means you can migrate_cancel
even in that situation.  The same thing happens when you're using
sockets in both directions, if you get an error on one direction you can
shutdown(2) the other direction so you know any thread doesn't get stuck
on it.

Dave

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