On Thu, Jan 9, 2020 at 11:35 AM Xavi Hernandez <jaher...@redhat.com> wrote:
> On Thu, Jan 9, 2020 at 10:22 AM Amar Tumballi <ama...@gmail.com> wrote: > >> >> >> On Thu, Jan 9, 2020 at 2:33 PM Xavi Hernandez <jaher...@redhat.com> >> wrote: >> >>> On Thu, Jan 9, 2020 at 9:44 AM Amar Tumballi <ama...@gmail.com> wrote: >>> >>>> >>>> >>>> On Thu, Jan 9, 2020 at 1:38 PM Xavi Hernandez <jaher...@redhat.com> >>>> wrote: >>>> >>>>> On Sun, Dec 22, 2019 at 4:56 PM Yaniv Kaul <yk...@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@kadalu glusterfs]$ git grep extra_stdfree | wc -l >> 40 >> [amar@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, ¶ms);* 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@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@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@gluster.org https://lists.gluster.org/mailman/listinfo/gluster-devel