<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 9, 2020 at 11:35 AM Xavi Hernandez <<a href="mailto:jahernan@redhat.com">jahernan@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, Jan 9, 2020 at 10:22 AM Amar Tumballi <<a href="mailto:amarts@gmail.com" target="_blank">amarts@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 9, 2020 at 2:33 PM Xavi Hernandez <<a href="mailto:jahernan@redhat.com" target="_blank">jahernan@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, Jan 9, 2020 at 9:44 AM Amar Tumballi <<a href="mailto:amarts@gmail.com" target="_blank">amarts@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 9, 2020 at 1:38 PM Xavi Hernandez <<a href="mailto:jahernan@redhat.com" target="_blank">jahernan@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Sun, Dec 22, 2019 at 4:56 PM Yaniv Kaul <<a href="mailto:ykaul@redhat.com" target="_blank">ykaul@redhat.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div style="font-family:arial,helvetica,sans-serif">I could not find a relevant use for them. Can anyone enlighten me?</div></div></blockquote><div><br></div><div>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.</div></div></div></blockquote><div><br></div><div>Xavi,</div><div><br></div><div>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. </div><div><br></div><div>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.</div></div></div></blockquote><div><br></div><div>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 ?</div><div><br></div><div>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.</div><div><br></div><div>Am I missing something else ?</div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>[amar@kadalu glusterfs]$ git grep extra_stdfree | wc -l<br>40<br>[amar@kadalu glusterfs]$ git grep extra_free | wc -l<br>5<br></div></div></div></blockquote><div><br></div><div>Yes, they call dict_unserialize() and then store the unserialized data into those variables. That's what I'm saying it's not necessary.</div></div></div></blockquote><div><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">In at least 2 cases, there's even something stranger I could not understand (see in bold - from server_setvolume() function) :</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><b> params = dict_new();</b><br> reply = dict_new();<br> ret = xdr_to_generic(req->msg[0], &args, (xdrproc_t)xdr_gf_setvolume_req);<br> if (ret < 0) {<br> // failed to decode msg;<br> req->rpc_err = GARBAGE_ARGS;<br> goto fail;<br> }<br> ctx = THIS->ctx;<br><br> this = req->svc->xl;<br> /* this is to ensure config_params is populated with the first brick<br> * details at first place if brick multiplexing is enabled<br> */<br> config_params = dict_copy_with_ref(this->options, NULL);<br><br><b> buf = gf_memdup(args.dict.dict_val, args.dict.dict_len);</b><br> if (buf == NULL) {<br> op_ret = -1;<br> op_errno = ENOMEM;<br> goto fail;<br> }<br><br><b> ret = dict_unserialize(buf, args.dict.dict_len, &params);</b><br> if (ret < 0) {<br> ret = dict_set_str(reply, "ERROR",<br> "Internal error: failed to unserialize "<br> "request dictionary");<br> if (ret < 0)<br> gf_msg_debug(this->name, 0,<br> "failed to set error "<br> "msg \"%s\"",<br> "Internal error: failed "<br> "to unserialize request dictionary");<br><br> op_ret = -1;<br> op_errno = EINVAL;<br> goto fail;<br> }<br><br><b> params->extra_free = buf;<br> buf = NULL;<br></b></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>I think this is not needed anymore. Probably we could remove these fields if that's the only reason.</div></div></div></blockquote><div><br></div><div>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).</div><div><br></div><div>Regards,</div><div>Amar</div><div>---</div><div><a href="https://kadalu.io" target="_blank">https://kadalu.io</a></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div style="font-family:arial,helvetica,sans-serif">TIA,</div><div style="font-family:arial,helvetica,sans-serif">Y.<br></div></div>
_______________________________________________<br>
<br>
Community Meeting Calendar:<br>
<br>
APAC Schedule -<br>
Every 2nd and 4th Tuesday at 11:30 AM IST<br>
Bridge: <a href="https://bluejeans.com/441850968" rel="noreferrer" target="_blank">https://bluejeans.com/441850968</a><br>
<br>
<br>
NA/EMEA Schedule -<br>
Every 1st and 3rd Tuesday at 01:00 PM EDT<br>
Bridge: <a href="https://bluejeans.com/441850968" rel="noreferrer" target="_blank">https://bluejeans.com/441850968</a><br>
<br>
Gluster-devel mailing list<br>
<a href="mailto:Gluster-devel@gluster.org" target="_blank">Gluster-devel@gluster.org</a><br>
<a href="https://lists.gluster.org/mailman/listinfo/gluster-devel" rel="noreferrer" target="_blank">https://lists.gluster.org/mailman/listinfo/gluster-devel</a><br>
<br>
</blockquote></div></div>
_______________________________________________<br>
<br>
Community Meeting Calendar:<br>
<br>
APAC Schedule -<br>
Every 2nd and 4th Tuesday at 11:30 AM IST<br>
Bridge: <a href="https://bluejeans.com/441850968" rel="noreferrer" target="_blank">https://bluejeans.com/441850968</a><br>
<br>
<br>
NA/EMEA Schedule -<br>
Every 1st and 3rd Tuesday at 01:00 PM EDT<br>
Bridge: <a href="https://bluejeans.com/441850968" rel="noreferrer" target="_blank">https://bluejeans.com/441850968</a><br>
<br>
Gluster-devel mailing list<br>
<a href="mailto:Gluster-devel@gluster.org" target="_blank">Gluster-devel@gluster.org</a><br>
<a href="https://lists.gluster.org/mailman/listinfo/gluster-devel" rel="noreferrer" target="_blank">https://lists.gluster.org/mailman/listinfo/gluster-devel</a><br>
<br>
</blockquote></div></div>
</blockquote></div></div>
</blockquote></div></div>
</blockquote></div></div>
</blockquote></div></div>