Re: [PATCH v3 19/21] migration: Add zlib compression multifd support

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---

>> +    /* We will never have more than page_count pages */
>> +    z->zbuff_len = page_count * qemu_target_page_size();
>> +    z->zbuff_len *= 2;
>> +    z->zbuff = g_try_malloc(z->zbuff_len);
>> +    if (!z->zbuff) {
>
> Does a deflateEnd need to be called here?

Shouldnt matter, but I think that yes.

>
>> +        g_free(z);
>> +        error_setg(errp, "multifd %d: out of memory for zbuff", p->id);
>> +        return -1;
>> +    }
>> +    return 0;
>
> I'd like to understand more aobut the failure path - lets say we exit
> through one of those return -1's, p->data is still set to point to z
> which is now been free'd.  Will zlib_send_cleanup get called?
> Maybe it's safer to move the 'p->data = z' to right at the bottom before
> the return 0 ?

Just did that. Good catch.

>
>> +}
>> +
>> +/**
>> + * zlib_send_cleanup: cleanup send side
>> + *
>> + * Close the channel and return memory.
>> + *
>> + * @p: Params for the channel that we are using
>> + */
>> +static void zlib_send_cleanup(MultiFDSendParams *p, Error **errp)
>> +{
>> +    struct zlib_data *z = p->data;
>
> As previously asked above, could this ever get called if zlib_send_setup
> has failed? If so does this need to check for !z ?

No.
if multifd_zlib_setup() returns != 0, then multifd_save_setup() returns
!= 0, and then we just signal an error and then we cancel migration in
multifd_fd_connect.

So, if z is NULL, we are already in big trouble.

>> +
>> +    for (i = 0; i < used; i++) {
>> +        uint32_t available = z->zbuff_len - out_size;
>> +        int flush = Z_NO_FLUSH;
>> +
>> +        if (i == used  - 1) {
>
> Odd double space formatting there.

Ouch.  Changed.
>> +    /* We will never have more than page_count pages */
>> +    z->zbuff_len = page_count * qemu_target_page_size();
>> +    /* We know compression "could" use more space */
>> +    z->zbuff_len *= 2;
>> +    z->zbuff = g_try_malloc(z->zbuff_len);
>> +    if (!z->zbuff) {
>
> inflateEnd and similar question to save?

Done.

>> +    for (i = 0; i < used; i++) {
>> +        struct iovec *iov = &p->pages->iov[i];
>> +        int flush = Z_NO_FLUSH;
>> +
>> +        if (i == used  - 1) {
>> +            flush = Z_SYNC_FLUSH;
>> +        }
>> +
>> +        zs->avail_out = iov->iov_len;
>> +        zs->next_out = iov->iov_base;
>> +
>> +        ret = inflate(zs, flush);
>> +        if (ret != Z_OK) {
>> +            error_setg(errp, "multifd %d: inflate returned %d instead of Z_OK",
>> +                       p->id, ret);
>> +            return ret;
>> +        }
>> +        out_size += iov->iov_len;
>
> How do we know that's iov_len ?

Because we never had fails O:-)
(If it is not, we have a corrupted stream or something has gone very
wrong).

You win.  I think that the correct value is:

uint32_t out_size = zs->total_out;
...

out_size += zs->total_out - out_size;

As a bonus, we can do that outside of the loop.

Thanks, Juan.