[Gluster-devel] What do extra_free and extrastd_free params do in the dictionary object?

[Yaniv Kaul]

On Thu, Jan 9, 2020 at 11:35 AM Xavi Hernandez <jahernan at redhat.com> wrote:

> On Thu, Jan 9, 2020 at 10:22 AM Amar Tumballi <amarts at gmail.com> wrote:
>
>>
>>
>> On Thu, Jan 9, 2020 at 2:33 PM Xavi Hernandez <jahernan at redhat.com>
>> wrote:
>>
>>> On Thu, Jan 9, 2020 at 9:44 AM Amar Tumballi <amarts at gmail.com> wrote:
>>>
>>>>
>>>>
>>>> On Thu, Jan 9, 2020 at 1:38 PM Xavi Hernandez <jahernan at redhat.com>
>>>> wrote:
>>>>
>>>>> On Sun, Dec 22, 2019 at 4:56 PM Yaniv Kaul <ykaul at redhat.com> wrote:
>>>>>
>>>>>> I could not find a relevant use for them. Can anyone enlighten me?
>>>>>>
>>>>>
>>>>> I'm not sure why they are needed. They seem to be used to keep the
>>>>> unserialized version of a dict around until the dict is destroyed. I
>>>>> thought this could be because we were using pointers to the unserialized
>>>>> data inside dict, but that's not the case currently. However, checking very
>>>>> old versions (pre 3.2), I see that dict values were not allocated, but a
>>>>> pointer to the unserialized data was used.
>>>>>
>>>>
>>>> Xavi,
>>>>
>>>> While you are right about the intent, it is used still, at least when I
>>>> grepped latest repo to keep a reference in protocol layer.
>>>>
>>>> This is done to reduce a copy after the dictionary's binary content is
>>>> received from RPC. The 'extra_free' flag is used when we use a
>>>> GF_*ALLOC()'d buffer in protocol to receive dictionary, and extra_stdfree
>>>> is used when RPC itself allocates the buffer and hence uses 'free()' to
>>>> free the buffer.
>>>>
>>>
>>> I don't see it. When dict_unserialize() is called, key and value are
>>> allocated and copied, so  why do we need to keep the raw data after that ?
>>>
>>> In 3.1 the value was simply a pointer to the unserialized data, but
>>> starting with 3.2, value is memdup'ed. Key is always copied. I don't see
>>> any other reference to the unserialized data right now. I think that
>>> instead of assigning the raw data to extra_(std)free, we should simply
>>> release that memory and remove those fields.
>>>
>>> Am I missing something else ?
>>>
>>
>> I did grep on 'extra_stdfree' and 'extra_free' and saw that many
>> handshake/ and protocol code seemed to use it. Haven't gone deeper to check
>> which part.
>>
>> [amar at kadalu glusterfs]$ git grep extra_stdfree | wc -l
>> 40
>> [amar at kadalu glusterfs]$ git grep extra_free | wc -l
>> 5
>>
>
> Yes, they call dict_unserialize() and then store the unserialized data
> into those variables. That's what I'm saying it's not necessary.
>

In at least 2 cases, there's even something stranger I could not understand
(see in bold - from server_setvolume() function) :
*    params = dict_new();*
    reply = dict_new();
    ret = xdr_to_generic(req->msg[0], &args,
(xdrproc_t)xdr_gf_setvolume_req);
    if (ret < 0) {
        // failed to decode msg;
        req->rpc_err = GARBAGE_ARGS;
        goto fail;
    }
    ctx = THIS->ctx;

    this = req->svc->xl;
    /* this is to ensure config_params is populated with the first brick
     * details at first place if brick multiplexing is enabled
     */
    config_params = dict_copy_with_ref(this->options, NULL);

*    buf = gf_memdup(args.dict.dict_val, args.dict.dict_len);*
    if (buf == NULL) {
        op_ret = -1;
        op_errno = ENOMEM;
        goto fail;
    }

*    ret = dict_unserialize(buf, args.dict.dict_len, &params);*
    if (ret < 0) {
        ret = dict_set_str(reply, "ERROR",
                           "Internal error: failed to unserialize "
                           "request dictionary");
        if (ret < 0)
            gf_msg_debug(this->name, 0,
                         "failed to set error "
                         "msg \"%s\"",
                         "Internal error: failed "
                         "to unserialize request dictionary");

        op_ret = -1;
        op_errno = EINVAL;
        goto fail;
    }



*    params->extra_free = buf;    buf = NULL;*

>
>
>>
>>>
>>>
>>>>
>>>>> I think this is not needed anymore. Probably we could remove these
>>>>> fields if that's the only reason.
>>>>>
>>>>
>>>> If keeping them is hard to maintain, we can add few allocation to
>>>> remove those elements, that shouldn't matter much IMO too. We are not using
>>>> dictionary itself as protocol now (which we did in 1.x series though).
>>>>
>>>> Regards,
>>>> Amar
>>>> ---
>>>> https://kadalu.io
>>>>
>>>>
>>>>
>>>>> TIA,
>>>>>> Y.
>>>>>> _______________________________________________
>>>>>>
>>>>>> Community Meeting Calendar:
>>>>>>
>>>>>> APAC Schedule -
>>>>>> Every 2nd and 4th Tuesday at 11:30 AM IST
>>>>>> Bridge: https://bluejeans.com/441850968
>>>>>>
>>>>>>
>>>>>> NA/EMEA Schedule -
>>>>>> Every 1st and 3rd Tuesday at 01:00 PM EDT
>>>>>> Bridge: https://bluejeans.com/441850968
>>>>>>
>>>>>> Gluster-devel mailing list
>>>>>> Gluster-devel at gluster.org
>>>>>> https://lists.gluster.org/mailman/listinfo/gluster-devel
>>>>>>
>>>>>> _______________________________________________
>>>>>
>>>>> Community Meeting Calendar:
>>>>>
>>>>> APAC Schedule -
>>>>> Every 2nd and 4th Tuesday at 11:30 AM IST
>>>>> Bridge: https://bluejeans.com/441850968
>>>>>
>>>>>
>>>>> NA/EMEA Schedule -
>>>>> Every 1st and 3rd Tuesday at 01:00 PM EDT
>>>>> Bridge: https://bluejeans.com/441850968
>>>>>
>>>>> Gluster-devel mailing list
>>>>> Gluster-devel at gluster.org
>>>>> https://lists.gluster.org/mailman/listinfo/gluster-devel
>>>>>
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20200109/00dd0742/attachment-0001.html>