Re: [Qemu-devel] [PATCH v3 8/8] multifd: rest of zlib compression

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> This is still a work in progress, but get everything sent as expected
>> and it is faster than the code that is already there.
>> 
>> Signed-off-by: Juan Quintela <address@hidden>
>> ---
>>  migration/ram.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 104 insertions(+), 2 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index fdb5bf07a5..efbb253c1a 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -747,6 +747,100 @@ MultifdMethods multifd_none_ops = {
>>      .recv_pages = none_recv_pages
>>  };
>>  
>> +/* Multifd zlib compression */
>> +
>
> Comment the return value?

Once there, commented all the functions.

>> +static int zlib_send_prepare(MultiFDSendParams *p, uint32_t used)
>> +{
>> +    struct iovec *iov = p->pages->iov;
>> +    z_stream *zs = &p->zs;
>> +    uint32_t out_size = 0;
>> +    int ret;
>> +    int i;
>
> uint32_t to match 'used' ?

Done

>> +    for (i = 0; i < used; i++) {
>> +        uint32_t available = p->zbuff_len - out_size;
>> +        int flush = Z_NO_FLUSH;
>> +
>> +        if (i == used  - 1) {
>> +            flush = Z_SYNC_FLUSH;
>> +        }
>> +
>> +        zs->avail_in = iov[i].iov_len;
>> +        zs->next_in = iov[i].iov_base;
>> +
>> +        zs->avail_out = available;
>> +        zs->next_out = p->zbuff + out_size;
>> +
>> +        ret = deflate(zs, flush);
>> +        if (ret != Z_OK) {
>> +            printf("problem with deflate? %d\n", ret);
>
> If it's an error it should probably be at least an fprintf(stderr or
> err_ something.

We don't have any error arround really, we need one. Searching for it.

> Should this also check that the avail_in/next_in has consumed the whole
> of the input?

I am not checking because _it_ is supposed to b doing it right.  We can
test it through, specially in reception.

>> +            qemu_mutex_unlock(&p->mutex);
>
> Can you explain and/or comment whyit's unlocked here in the error path?

Uh, oh ....

Leftover for when it was done inline inside the main function.
Removed.

>> +            return -1;
>> +        }
>> +        out_size += available - zs->avail_out;
>> +    }
>> +    p->next_packet_size = out_size;
>
> Some traces_ wouldn't hurt.

Humm, you are right here.

Thanks, Juan.