"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> Signed-off-by: Juan Quintela <quint...@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.
