Re: [Qemu-devel] [PATCH 4/5] qemu-file: Fix qemu_put_compression_data flaw

[Li, Liang Z]

> Liang Li <address@hidden> wrote:
> > Current qemu_put_compression_data can only work with no writable
> > QEMUFile, and can't work with the writable QEMUFile. But it does not
> > provide any measure to prevent users from using it with a writable
> > QEMUFile.
> >
> > We should fix this flaw to make it works with writable QEMUFile.
> >
> > Suggested-by: Juan Quintela <address@hidden>
> > Signed-off-by: Liang Li <address@hidden>
> 
> Code is not enough.
> 
> > ---
> >  migration/qemu-file.c | 23 +++++++++++++++++++++--
> >  migration/ram.c       |  6 +++++-
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c index
> > 6f4a129..b0ef1f3 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f)
> >      return v;
> >  }
> >
> > -/* compress size bytes of data start at p with specific compression
> > +/* Compress size bytes of data start at p with specific compression
> >   * level and store the compressed data to the buffer of f.
> > + *
> > + * When f is not writable, return -1 if f has no space to save the
> > + * compressed data.
> > + * When f is wirtable and it has no space to save the compressed
> > + data,
> > + * do fflush first, if f still has no space to save the compressed
> > + * data, return -1.
> >   */
> >
> >  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p,
> > size_t size, @@ -617,7 +623,14 @@ ssize_t
> qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
> >      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
> >
> >      if (blen < compressBound(size)) {
> > -        return 0;
> > +        if (!qemu_file_is_writable(f)) {
> > +            return -1;
> > +        }
> 
> I can't yet understand why we can be writting to a qemu_file that is not
> writable.
> But well, no harm.
> 
> 
> > +        qemu_fflush(f);
> > +        blen = IO_BUF_SIZE - sizeof(int32_t);
> > +        if (blen < compressBound(size)) {
> > +            return -1;
> > +        }
> >      }
> >      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
> >                    (Bytef *)p, size, level) != Z_OK) { @@ -625,7
> > +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const
> uint8_t *p, size_t size,
> >          return 0;
> >      }
> >      qemu_put_be32(f, blen);
> > +    if (f->ops->writev_buffer) {
> > +        add_to_iovec(f, f->buf + f->buf_index, blen);
> > +    }
> 
> 
> 
> > +    }
> >      return blen + sizeof(int32_t);
> >  }
> >
> 
> Ok with the changes in this function.
> 
> 
> > diff --git a/migration/ram.c b/migration/ram.c index bc34bc5..7e62d8d
> > 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -821,7 +821,11 @@ static int
> do_compress_ram_page(CompressParam *param)
> >                                    RAM_SAVE_FLAG_COMPRESS_PAGE);
> >      blen = qemu_put_compression_data(param->file, p,
> TARGET_PAGE_SIZE,
> >                                       migrate_compress_level());
> > -    bytes_sent += blen;
> > +    if (blen < 0) {
> > +        error_report("Insufficient buffer for compressed data!");
> > +    } else {
> > +        bytes_sent += blen;
> > +    }
> >
> >      return bytes_sent;
> >  }
> 
> 
> Are you sure that this code is ok?

No.
> 
> 1st- there are two callers od do_compress_ram_page(), only one of it
>      uses the return value
> 2nd- if we were not able to write the compressed page, we continue
>      without a single error.  I think that if blen is < 0, we should
>      just abort the migration at this point (we have just sent a header
>      and we haven't been able to send the contents under that header.
>      Without lot of changes, I can't see how to fix it).
> 

Abort the migration is the right choice,  use qemu_file_set_error() to set the 
error flag is enough? 
Thanks

Liang

> Thanks, Juan.