9:08 p.m.
On Tue, Jun 29, 2021 at 11:52 PM Solomon Boulos <boulos(a)google.com> wrote:
Alright, looking through some git blame -w, I think this broke and
then
was restored in 2912fad98e. Jim had a diff in 2014 (823625c5da) to make the
code compile, but crucially the buffer code no longer had a “max len” just
a length.
I think this broke the “clever” reuse of the input buffdesc as the
argument to GetFH (which has been there since 2012 from 878f45493e).
The input when mapped is a 16-byte thingy (sizeof nfs23_map_handle_t), and
GetFH needs to expand the 16-byte mapped handle into an FH4. I assume the
code when written had a FH4_SIZE as it’s max length, rather than the
fh_desc buffer appearing to be trimmed to 16-bytes (that is, I think there
was a length separate from max length at the time or something).
Digging into this a little more, I think the following commit actually
originally broke things:
3016c4cee45635afa9c25b21c2f5b014d80d1b6d
In that commit, the "netbuf" data type used as an argument
for HandleMap_GetFH is replaced with the "gsh_buffdesc" data type. As far
as I can tell, "gsh_buffdesc" never had a "maxlen" member, so the
following
code in that function would not have compiled after that:
==
...
if(rc == HASHTABLE_SUCCESS)
{
handle_pool_entry_t *h = (handle_pool_entry_t *)buffval.pdata;
if(h->fh_len < p_out_fsal_handle->maxlen)
{
p_out_fsal_handle->len = h->fh_len;
memcpy(p_out_fsal_handle->buf, h->fh_data, h->fh_len);
memcpy(p_out_fsal_handle->addr, h->fh_data, h->fh_len);
rc = HANDLEMAP_SUCCESS;
}
...
==
After that change, there are various iterations of renaming the
"p_out_fsal_handle" parameter to other things, up until the commit that you
mentioned that fixed the compile issues
(823625c5da2373606dd4f75dce46a92be143507c), which resulted in the code
having "p_out_fsal_handle" renamed to the value actually being passed in,
and the "maxlen" member replaced with "len".
IIRC, all the handle mapping code *actually* passes around a buffer
that
is large enough for the largest FH4 plus some padding. So the “resizing”
done in handle_to_wire that needs to be undone in wire_to_host can actually
happen in-place but the len field doesn’t know that.
So, any suggestions on the most reliable way to determine "maxlen", again?
Should there be a #define constant somewhere that sets it, or is it
something that should be more dynamically computed based on the data passed
in - maybe the function that does the padding? Or should the "if ()"
statement be removed entirely, and just assume that the length is always
small enough (that seems like a bad option)?
Seems like there probably aren't a lot of users of the handle mapping code
:-D.
-Nick