Correct the fh returned when reusing a handle from external table

[smithdh]

This is a proposed fix for a problem seen with EOS; I suppose it's probably only possible with xrootd servers configured with plugins so that they use the delayed close feature (i.e. when the Ofs file close() can return SFS_STARTED).

I haven't opened an issue ticket for now (to describing what was seen, or attempts to reproduce the problem), but I could do so later if we want, but for now I won't. Essentially it seems the wrong file was closed or bytes were read from the wrong file.

This PR aims to correct the file handle number returned by XrdXrootdFileTable::Add() in case a heldSpot in the external table gets reused.

[amadio]

The fix looks good to me. However, It looks like the logic handling the FTab and XTab tables here is quite error-prone, so we should consider refactoring this a bit in the future. For example, rather than modifying i in place, we may call a find_file_handle(i, FTab), then if that fails, call find_file_handle(i - XRD_FTABSIZE, XTab) or something like that. We could also move from malloc/free to a std::array for FTab (since it seems to have a compile-time fixed size), and a std::vector for the XTab. At the very least, we could try using realloc to extend XTab. @abh What are your thoughts on this?

[amadio]

Suggested change

	else {i -= XRD_FTABSIZE;
	if (XTab && i < XTnum) fP = &XTab[i];
	else fP = 0;
	i += XRD_FTABSIZE;
	}
	else {int ix = i - XRD_FTABSIZE;
	if (XTab && ix < XTnum) fP = &XTab[ix];
	else fP = 0;
	}

[abh3]

Well, I willlook at this more closely but simply looking at the small code segment it's quite obvious that he logic is completely different between the two. Si, I say caution!

…

On Mon, 17 Apr 2023, Guilherme Amadio wrote: @amadio commented on this pull request. > else {i -= XRD_FTABSIZE; if (XTab && i < XTnum) fP = &XTab[i]; else fP = 0; + i += XRD_FTABSIZE; } ```suggestion else {int ix = i - XRD_FTABSIZE; if (XTab && ix < XTnum) fP = &XTab[ix]; else fP = 0; } ``` -- Reply to this email directly or view it on GitHub: #1998 (review) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***>

[abh3]

I'm not quite sure what is error prone here as this has worked for decades and the particular issue is an edge case. Be ware that this object is one of the most used objects in xroot. It was orignally written to be super fast and use the minimum amount of memory. Accomplishing both does makes it more difficult to understand. However, given its position in the execution path I'd say leave it as is.

…

On Mon, 17 Apr 2023, Guilherme Amadio wrote: The fix looks good to me. However, It looks like the logic handling the `FTab` and `XTab` tables here is quite error-prone, so we should consider refactoring this a bit in the future. For example, rather than modifying `i` in place, we may call a `find_file_handle(i, FTab)`, then if that fails, call `find_file_handle(i - XRD_FTABSIZE, XTab)` or something like that. We could also move from malloc/free to a `std::array` for FTab (since it seems to have a compile-time fixed size), and a `std::vector` for the `XTab`. @abh What are your thoughts on this? -- Reply to this email directly or view it on GitHub: #1998 (comment) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***>

[amadio]

Ok, I'm merging it then.