All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Daniel P. Berrange" <berrange@redhat.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Amit Shah <amit.shah@redhat.com>,
	qemu-devel@nongnu.org, Juan Quintela <quintela@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v1 03/22] migration: ensure qemu_fflush() always writes full data amount
Date: Wed, 3 Feb 2016 13:31:46 +0000	[thread overview]
Message-ID: <20160203133146.GL30222@redhat.com> (raw)
In-Reply-To: <20160128175346.GC2911@work-vm>

On Thu, Jan 28, 2016 at 05:53:46PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > The QEMUFile writev_buffer / put_buffer functions are expected
> > to write out the full set of requested data, blocking until
> > complete. The qemu_fflush() caller does not expect to deal with
> > partial writes. Clarify the function comments and add a sanity
> > check to the code to catch mistaken implementations.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  include/migration/qemu-file.h |  6 ++++--
> >  migration/qemu-file.c         | 16 ++++++++++++----
> >  2 files changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> > index b5d08d2..5debe8c 100644
> > --- a/include/migration/qemu-file.h
> > +++ b/include/migration/qemu-file.h
> > @@ -29,7 +29,8 @@
> >  
> >  /* This function writes a chunk of data to a file at the given position.
> >   * The pos argument can be ignored if the file is only being used for
> > - * streaming.  The handler should try to write all of the data it can.
> > + * streaming.  The handler must write all of the data or return a negative
> > + * errno value.
> >   */
> >  typedef ssize_t (QEMUFilePutBufferFunc)(void *opaque, const uint8_t *buf,
> >                                          int64_t pos, size_t size);
> > @@ -55,7 +56,8 @@ typedef int (QEMUFileCloseFunc)(void *opaque);
> >  typedef int (QEMUFileGetFD)(void *opaque);
> >  
> >  /*
> > - * This function writes an iovec to file.
> > + * This function writes an iovec to file. The handler must write all
> > + * of the data or return a negative errno value.
> >   */
> >  typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
> >                                             int iovcnt, int64_t pos);
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index 0bbd257..f89e64e 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -107,11 +107,13 @@ bool qemu_file_is_writable(QEMUFile *f)
> >   * Flushes QEMUFile buffer
> >   *
> >   * If there is writev_buffer QEMUFileOps it uses it otherwise uses
> > - * put_buffer ops.
> > + * put_buffer ops. This will flush all pending data. If data was
> > + * only partially flushed, it will set an error state.
> >   */
> >  void qemu_fflush(QEMUFile *f)
> >  {
> >      ssize_t ret = 0;
> > +    ssize_t expect = 0;
> >  
> >      if (!qemu_file_is_writable(f)) {
> >          return;
> > @@ -119,21 +121,27 @@ void qemu_fflush(QEMUFile *f)
> >  
> >      if (f->ops->writev_buffer) {
> >          if (f->iovcnt > 0) {
> > +            expect = iov_size(f->iov, f->iovcnt);
> >              ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos);
> >          }
> >      } else {
> >          if (f->buf_index > 0) {
> > +            expect = f->buf_index;
> >              ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
> >          }
> >      }
> > +
> >      if (ret >= 0) {
> >          f->pos += ret;
> >      }
> > -    f->buf_index = 0;
> > -    f->iovcnt = 0;
> > -    if (ret < 0) {
> > +    /* We expect the QEMUFile write impl to send the full
> > +     * data set we requested, so sanity check that.
> > +     */
> > +    if (ret < 0 || ret != expect) {
> >          qemu_file_set_error(f, ret);
> 
> You could simplify that to     if (ret != expect)     couldn't you?
> 
> In the case you're trying to guard against, the value past
> to qemu_file_set_error is potentially truncated; which in the worst
> case could make it appear as success; although I doubt that
> can happen in our uses.

qemu_file_set_error expects an errni, so in fact my code was
buggy if the ret != expect part got evaluated, so I'll change
this to

     if (ret != expect) {
        qemu_file_set_error(f, ret < 0 ? ret : -EIO);
     }


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

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

  reply	other threads:[~2016-02-03 13:32 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-12 11:43 [Qemu-devel] [PATCH v1 00/22] Convert migration to QIOChannel & support TLS Daniel P. Berrange
2016-01-12 11:43 ` [Qemu-devel] [PATCH v1 01/22] s390: use FILE instead of QEMUFile for creating text file Daniel P. Berrange
2016-01-12 11:58   ` Cornelia Huck
2016-01-12 12:01     ` Daniel P. Berrange
2016-01-12 12:05       ` Cornelia Huck
2016-02-12 17:19   ` Dr. David Alan Gilbert
2016-01-12 11:43 ` [Qemu-devel] [PATCH v1 02/22] migration: remove use of qemu_bufopen from vmstate tests Daniel P. Berrange
2016-01-28 17:45   ` Dr. David Alan Gilbert
2016-01-12 11:43 ` [Qemu-devel] [PATCH v1 03/22] migration: ensure qemu_fflush() always writes full data amount Daniel P. Berrange
2016-01-28 17:53   ` Dr. David Alan Gilbert
2016-02-03 13:31     ` Daniel P. Berrange [this message]
2016-01-12 11:43 ` [Qemu-devel] [PATCH v1 04/22] migration: split migration hooks out of QEMUFileOps Daniel P. Berrange
2016-01-28 17:57   ` Dr. David Alan Gilbert
2016-01-12 11:43 ` [Qemu-devel] [PATCH v1 05/22] migration: introduce set_blocking function in QEMUFileOps Daniel P. Berrange
2016-01-28 18:00   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 06/22] migration: force QEMUFile to blocking mode for outgoing migration Daniel P. Berrange
2016-01-28 18:17   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 07/22] migration: introduce a new QEMUFile impl based on QIOChannel Daniel P. Berrange
2016-02-02 17:06   ` Dr. David Alan Gilbert
2016-02-03 13:37     ` Daniel P. Berrange
2016-02-12 17:16       ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 08/22] migration: convert post-copy to use QIOChannelBuffer Daniel P. Berrange
2016-01-25 19:38   ` Dr. David Alan Gilbert
2016-01-25 22:15     ` Daniel P. Berrange
2016-01-26 18:59       ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 09/22] migration: convert unix socket protocol to use QIOChannel Daniel P. Berrange
2016-02-02 18:02   ` Dr. David Alan Gilbert
2016-02-03 11:25     ` Daniel P. Berrange
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 10/22] migration: convert tcp " Daniel P. Berrange
2016-02-02 18:19   ` Dr. David Alan Gilbert
2016-02-03 10:02     ` Daniel P. Berrange
2016-02-03 10:33       ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 11/22] migration: convert fd " Daniel P. Berrange
2016-02-02 18:46   ` Dr. David Alan Gilbert
2016-02-03 10:05     ` Daniel P. Berrange
2016-02-03 10:29       ` Dr. David Alan Gilbert
2016-02-03 10:39         ` Daniel P. Berrange
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 12/22] migration: convert exec " Daniel P. Berrange
2016-02-02 18:53   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 13/22] migration: convert RDMA to use QIOChannel interface Daniel P. Berrange
2016-02-02 20:01   ` Dr. David Alan Gilbert
2016-02-03 11:37     ` Daniel P. Berrange
2016-02-03 13:23       ` Dr. David Alan Gilbert
2016-02-03 13:25         ` Daniel P. Berrange
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 14/22] migration: convert savevm to use QIOChannel for writing to files Daniel P. Berrange
2016-02-03  9:52   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 15/22] migration: delete QEMUFile buffer implementation Daniel P. Berrange
2016-02-03  9:54   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 16/22] migration: delete QEMUSizedBuffer struct Daniel P. Berrange
2016-02-03  9:55   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 17/22] migration: delete QEMUFile sockets implementation Daniel P. Berrange
2016-02-03  9:56   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 18/22] migration: delete QEMUFile stdio implementation Daniel P. Berrange
2016-02-03  9:58   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 19/22] migration: move definition of struct QEMUFile back into qemu-file.c Daniel P. Berrange
2016-02-05 18:32   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 20/22] migration: support TLS encryption with TCP migration backend Daniel P. Berrange
2016-02-12 17:09   ` Dr. David Alan Gilbert
2016-02-12 17:25     ` Daniel P. Berrange
2016-02-15 11:00       ` Daniel P. Berrange
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 21/22] migration: remove support for non-iovec based write handlers Daniel P. Berrange
2016-02-12 15:48   ` Dr. David Alan Gilbert
2016-01-12 11:44 ` [Qemu-devel] [PATCH v1 22/22] migration: remove qemu_get_fd method from QEMUFile Daniel P. Berrange
2016-02-12 15:51   ` Dr. David Alan Gilbert
2016-01-12 11:59 ` [Qemu-devel] [PATCH v1 00/22] Convert migration to QIOChannel & support TLS Daniel P. Berrange
2016-01-20 18:01 ` Daniel P. Berrange

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160203133146.GL30222@redhat.com \
    --to=berrange@redhat.com \
    --cc=amit.shah@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.