1. Patchew
  2. QEMU
  3. [PATCH 00/20] migration: remove QEMUFileOps concept and assume use of QIOChannel
  4. View patch

[PATCH 05/20] migration: rename 'pos' field in QEMUFile to 'bytes_processed'

Daniel P. Berrangé posted 20 patches 3 years, 1 month ago
There is a newer version of this series
[PATCH 05/20] migration: rename 'pos' field in QEMUFile to 'bytes_processed'
Posted by Daniel P. Berrangé 3 years, 1 month ago 
This makes the field name align with the newly introduced method
names in the previous commit.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 migration/qemu-file.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 03f0b13a55..b21da4c5bf 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -50,8 +50,9 @@ struct QEMUFile {
      */
     int64_t rate_limit_used;
 
-    int64_t pos; /* start of buffer when writing, end of buffer
-                    when reading */
+    /* The sum of bytes transferred on the wire */
+    int64_t total_transferred;
+
     int buf_index;
     int buf_size; /* 0 when writing */
     uint8_t buf[IO_BUF_SIZE];
@@ -241,14 +242,14 @@ void qemu_fflush(QEMUFile *f)
     }
     if (f->iovcnt > 0) {
         expect = iov_size(f->iov, f->iovcnt);
-        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
+        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->total_transferred,
                                     &local_error);
 
         qemu_iovec_release_ram(f);
     }
 
     if (ret >= 0) {
-        f->pos += ret;
+        f->total_transferred += ret;
     }
     /* We expect the QEMUFile write impl to send the full
      * data set we requested, so sanity check that.
@@ -357,11 +358,11 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
         return 0;
     }
 
-    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
+    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->total_transferred,
                              IO_BUF_SIZE - pending, &local_error);
     if (len > 0) {
         f->buf_size += len;
-        f->pos += len;
+        f->total_transferred += len;
     } else if (len == 0) {
         qemu_file_set_error_obj(f, -EIO, local_error);
     } else if (len != -EAGAIN) {
@@ -375,7 +376,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
 
 void qemu_update_position(QEMUFile *f, size_t size)
 {
-    f->pos += size;
+    f->total_transferred += size;
 }
 
 /** Closes the file
@@ -658,7 +659,7 @@ int qemu_get_byte(QEMUFile *f)
 
 int64_t qemu_ftell_fast(QEMUFile *f)
 {
-    int64_t ret = f->pos;
+    int64_t ret = f->total_transferred;
     int i;
 
     for (i = 0; i < f->iovcnt; i++) {
@@ -671,7 +672,7 @@ int64_t qemu_ftell_fast(QEMUFile *f)
 int64_t qemu_ftell(QEMUFile *f)
 {
     qemu_fflush(f);
-    return f->pos;
+    return f->total_transferred;
 }
 
 int qemu_file_rate_limit(QEMUFile *f)
-- 
2.36.1
Re: [PATCH 05/20] migration: rename 'pos' field in QEMUFile to 'bytes_processed'
Posted by Dr. David Alan Gilbert 3 years, 1 month ago 
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> This makes the field name align with the newly introduced method
> names in the previous commit.

I think that's the method in the following commits?

tbh I'm not sure about this; 'pos' is still passed to writev_buffer
and get_buffer to say where the data is - and that makes it a 'pos'
still rather than a simple stats counter.

Dave

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  migration/qemu-file.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 03f0b13a55..b21da4c5bf 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -50,8 +50,9 @@ struct QEMUFile {
>       */
>      int64_t rate_limit_used;
>  
> -    int64_t pos; /* start of buffer when writing, end of buffer
> -                    when reading */
> +    /* The sum of bytes transferred on the wire */
> +    int64_t total_transferred;
> +
>      int buf_index;
>      int buf_size; /* 0 when writing */
>      uint8_t buf[IO_BUF_SIZE];
> @@ -241,14 +242,14 @@ void qemu_fflush(QEMUFile *f)
>      }
>      if (f->iovcnt > 0) {
>          expect = iov_size(f->iov, f->iovcnt);
> -        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
> +        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->total_transferred,
>                                      &local_error);
>  
>          qemu_iovec_release_ram(f);
>      }
>  
>      if (ret >= 0) {
> -        f->pos += ret;
> +        f->total_transferred += ret;
>      }
>      /* We expect the QEMUFile write impl to send the full
>       * data set we requested, so sanity check that.
> @@ -357,11 +358,11 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
>          return 0;
>      }
>  
> -    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
> +    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->total_transferred,
>                               IO_BUF_SIZE - pending, &local_error);
>      if (len > 0) {
>          f->buf_size += len;
> -        f->pos += len;
> +        f->total_transferred += len;
>      } else if (len == 0) {
>          qemu_file_set_error_obj(f, -EIO, local_error);
>      } else if (len != -EAGAIN) {
> @@ -375,7 +376,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
>  
>  void qemu_update_position(QEMUFile *f, size_t size)
>  {
> -    f->pos += size;
> +    f->total_transferred += size;
>  }
>  
>  /** Closes the file
> @@ -658,7 +659,7 @@ int qemu_get_byte(QEMUFile *f)
>  
>  int64_t qemu_ftell_fast(QEMUFile *f)
>  {
> -    int64_t ret = f->pos;
> +    int64_t ret = f->total_transferred;
>      int i;
>  
>      for (i = 0; i < f->iovcnt; i++) {
> @@ -671,7 +672,7 @@ int64_t qemu_ftell_fast(QEMUFile *f)
>  int64_t qemu_ftell(QEMUFile *f)
>  {
>      qemu_fflush(f);
> -    return f->pos;
> +    return f->total_transferred;
>  }
>  
>  int qemu_file_rate_limit(QEMUFile *f)
> -- 
> 2.36.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Re: [PATCH 05/20] migration: rename 'pos' field in QEMUFile to 'bytes_processed'
Posted by Daniel P. Berrangé 3 years, 1 month ago 
On Thu, Jun 09, 2022 at 10:51:27AM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > This makes the field name align with the newly introduced method
> > names in the previous commit.
> 
> I think that's the method in the following commits?

Opps, yeah, I did re-arrange this series a few times to get the
most attractive diffs.

> tbh I'm not sure about this; 'pos' is still passed to writev_buffer
> and get_buffer to say where the data is - and that makes it a 'pos'
> still rather than a simple stats counter.

Note every QIOChannel backed impl of QEMUFile is ignoring the
'pos' field.

The only QEMUFile impl using 'pos' is the one during I/O is
the block device vmstate. A later patch is introducing a
QIOChannel impl for the vmstate, and to handle this it is
tracking a file offset itself internally to the QIOChannel
impl. So when we eliminate the QEMUFileOps callbacks later,
this 'pos' goes away.

So I guess my description here is a little ahead of itself.


> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  migration/qemu-file.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index 03f0b13a55..b21da4c5bf 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -50,8 +50,9 @@ struct QEMUFile {
> >       */
> >      int64_t rate_limit_used;
> >  
> > -    int64_t pos; /* start of buffer when writing, end of buffer
> > -                    when reading */
> > +    /* The sum of bytes transferred on the wire */
> > +    int64_t total_transferred;
> > +
> >      int buf_index;
> >      int buf_size; /* 0 when writing */
> >      uint8_t buf[IO_BUF_SIZE];
> > @@ -241,14 +242,14 @@ void qemu_fflush(QEMUFile *f)
> >      }
> >      if (f->iovcnt > 0) {
> >          expect = iov_size(f->iov, f->iovcnt);
> > -        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
> > +        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->total_transferred,
> >                                      &local_error);
> >  
> >          qemu_iovec_release_ram(f);
> >      }
> >  
> >      if (ret >= 0) {
> > -        f->pos += ret;
> > +        f->total_transferred += ret;
> >      }
> >      /* We expect the QEMUFile write impl to send the full
> >       * data set we requested, so sanity check that.
> > @@ -357,11 +358,11 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> >          return 0;
> >      }
> >  
> > -    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
> > +    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->total_transferred,
> >                               IO_BUF_SIZE - pending, &local_error);
> >      if (len > 0) {
> >          f->buf_size += len;
> > -        f->pos += len;
> > +        f->total_transferred += len;
> >      } else if (len == 0) {
> >          qemu_file_set_error_obj(f, -EIO, local_error);
> >      } else if (len != -EAGAIN) {
> > @@ -375,7 +376,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> >  
> >  void qemu_update_position(QEMUFile *f, size_t size)
> >  {
> > -    f->pos += size;
> > +    f->total_transferred += size;
> >  }
> >  
> >  /** Closes the file
> > @@ -658,7 +659,7 @@ int qemu_get_byte(QEMUFile *f)
> >  
> >  int64_t qemu_ftell_fast(QEMUFile *f)
> >  {
> > -    int64_t ret = f->pos;
> > +    int64_t ret = f->total_transferred;
> >      int i;
> >  
> >      for (i = 0; i < f->iovcnt; i++) {
> > @@ -671,7 +672,7 @@ int64_t qemu_ftell_fast(QEMUFile *f)
> >  int64_t qemu_ftell(QEMUFile *f)
> >  {
> >      qemu_fflush(f);
> > -    return f->pos;
> > +    return f->total_transferred;
> >  }
> >  
> >  int qemu_file_rate_limit(QEMUFile *f)
> > -- 
> > 2.36.1
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH 05/20] migration: rename 'pos' field in QEMUFile to 'bytes_processed'
Posted by Dr. David Alan Gilbert 3 years, 1 month ago 
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Thu, Jun 09, 2022 at 10:51:27AM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > This makes the field name align with the newly introduced method
> > > names in the previous commit.
> > 
> > I think that's the method in the following commits?
> 
> Opps, yeah, I did re-arrange this series a few times to get the
> most attractive diffs.
> 
> > tbh I'm not sure about this; 'pos' is still passed to writev_buffer
> > and get_buffer to say where the data is - and that makes it a 'pos'
> > still rather than a simple stats counter.
> 
> Note every QIOChannel backed impl of QEMUFile is ignoring the
> 'pos' field.
> 
> The only QEMUFile impl using 'pos' is the one during I/O is
> the block device vmstate. A later patch is introducing a
> QIOChannel impl for the vmstate, and to handle this it is
> tracking a file offset itself internally to the QIOChannel
> impl. So when we eliminate the QEMUFileOps callbacks later,
> this 'pos' goes away.

Ah! Put that description in the commit message and:


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

> So I guess my description here is a little ahead of itself.
> 
> 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  migration/qemu-file.c | 19 ++++++++++---------
> > >  1 file changed, 10 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > > index 03f0b13a55..b21da4c5bf 100644
> > > --- a/migration/qemu-file.c
> > > +++ b/migration/qemu-file.c
> > > @@ -50,8 +50,9 @@ struct QEMUFile {
> > >       */
> > >      int64_t rate_limit_used;
> > >  
> > > -    int64_t pos; /* start of buffer when writing, end of buffer
> > > -                    when reading */
> > > +    /* The sum of bytes transferred on the wire */
> > > +    int64_t total_transferred;
> > > +
> > >      int buf_index;
> > >      int buf_size; /* 0 when writing */
> > >      uint8_t buf[IO_BUF_SIZE];
> > > @@ -241,14 +242,14 @@ void qemu_fflush(QEMUFile *f)
> > >      }
> > >      if (f->iovcnt > 0) {
> > >          expect = iov_size(f->iov, f->iovcnt);
> > > -        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
> > > +        ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->total_transferred,
> > >                                      &local_error);
> > >  
> > >          qemu_iovec_release_ram(f);
> > >      }
> > >  
> > >      if (ret >= 0) {
> > > -        f->pos += ret;
> > > +        f->total_transferred += ret;
> > >      }
> > >      /* We expect the QEMUFile write impl to send the full
> > >       * data set we requested, so sanity check that.
> > > @@ -357,11 +358,11 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> > >          return 0;
> > >      }
> > >  
> > > -    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
> > > +    len = f->ops->get_buffer(f->opaque, f->buf + pending, f->total_transferred,
> > >                               IO_BUF_SIZE - pending, &local_error);
> > >      if (len > 0) {
> > >          f->buf_size += len;
> > > -        f->pos += len;
> > > +        f->total_transferred += len;
> > >      } else if (len == 0) {
> > >          qemu_file_set_error_obj(f, -EIO, local_error);
> > >      } else if (len != -EAGAIN) {
> > > @@ -375,7 +376,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> > >  
> > >  void qemu_update_position(QEMUFile *f, size_t size)
> > >  {
> > > -    f->pos += size;
> > > +    f->total_transferred += size;
> > >  }
> > >  
> > >  /** Closes the file
> > > @@ -658,7 +659,7 @@ int qemu_get_byte(QEMUFile *f)
> > >  
> > >  int64_t qemu_ftell_fast(QEMUFile *f)
> > >  {
> > > -    int64_t ret = f->pos;
> > > +    int64_t ret = f->total_transferred;
> > >      int i;
> > >  
> > >      for (i = 0; i < f->iovcnt; i++) {
> > > @@ -671,7 +672,7 @@ int64_t qemu_ftell_fast(QEMUFile *f)
> > >  int64_t qemu_ftell(QEMUFile *f)
> > >  {
> > >      qemu_fflush(f);
> > > -    return f->pos;
> > > +    return f->total_transferred;
> > >  }
> > >  
> > >  int qemu_file_rate_limit(QEMUFile *f)
> > > -- 
> > > 2.36.1
> > > 
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.