Re: [Qemu-devel] [v3 04/13] qemu-file: Add tow function will be used in migration

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

* Liang Li (liang.z.li@intel.com) wrote:
> migrate_qemu_add_compression_data() compress the data
> and put it to QEMUFile. migrate_qemu_flush() put the
> data in the source QEMUFile to destination QEMUFile.
> 
> The two function can help to do live migration.

Typo in the title 'tow->two' - but perhaps a better title
would be 'Add compression functions to QEMUFile'

> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> ---
>  include/migration/qemu-file.h |  3 +++
>  qemu-file.c                   | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 401676b..d70fb8d 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -150,6 +150,9 @@ void qemu_put_be32(QEMUFile *f, unsigned int v);
>  void qemu_put_be64(QEMUFile *f, uint64_t v);
>  int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset);
>  int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size);
> +size_t migrate_qemu_add_compression_data(QEMUFile *f,
> +        const uint8_t *p, size_t size, int level);
> +int migrate_qemu_flush(QEMUFile *f_des, QEMUFile *f_src);
>  /*
>   * Note that you can only peek continuous bytes from where the current pointer
>   * is; you aren't guaranteed to be able to peak to +n bytes unless you've
> diff --git a/qemu-file.c b/qemu-file.c
> index f938e36..2668ad0 100644
> --- a/qemu-file.c
> +++ b/qemu-file.c
> @@ -21,6 +21,7 @@
>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>   * THE SOFTWARE.
>   */
> +#include <zlib.h>
>  #include "qemu-common.h"
>  #include "qemu/iov.h"
>  #include "qemu/sockets.h"
> @@ -993,3 +994,34 @@ QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
>      }
>      return s->file;
>  }
> +
> +/* compress size bytes of data start at p with specific compression
> + * leve and store the compressed data to the buffer of f.
> + */
> +
> +size_t migrate_qemu_add_compression_data(QEMUFile *f,
> +        const uint8_t *p, size_t size, int level)

It's an odd name, QEMUFile is only used by migration anyway;
maybe qemufile_add_compression_data ?

> +{
> +    size_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int);
> +
> +    if (compress2(f->buf + f->buf_index + sizeof(int), &blen, (Bytef *)p,
> +            size, level) != Z_OK) {
> +        error_report("Compress Failed!");
> +        return 0;
> +    }

What are the 'sizeof(int)'s for?  It's unusual because we normally keep the
format of the stream the same even if one side was a 32bit qemu and the other 64bit.
How do you know that there is enough space for the compress - i.e. what
happens if f->buf_index is too high?

> +    qemu_put_be32(f, blen);
> +    f->buf_index += blen;
> +    return blen + sizeof(int);
> +}
> +
> +int migrate_qemu_flush(QEMUFile *f_des, QEMUFile *f_src)
> +{
> +    int len = 0;
> +
> +    if (f_src->buf_index > 0) {
> +        len = f_src->buf_index;
> +        qemu_put_buffer(f_des, f_src->buf, f_src->buf_index);
> +        f_src->buf_index = 0;
> +    }
> +    return len;
> +}

Can you explain a bit more here how you're using the src file; I think
you're using it as kind of a dummy buffer; but it needs to be documented
somewhere.  Again I'm also not sure of the name of this function.

Dave

> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK