improve server readdir tracing#167
Conversation
Problem: the Npsrv->logmsg() callback is a printf style function, but protocol tracing requires the server implementation to print large, pre-formatted buffers. Convert the callback to a puts() style signature. Update the diod logmsg callback. Update unit tests that register libnpfs logmsg callbacks
grondo
left a comment
grondo
left a comment
There was a problem hiding this comment.
LGTM!
I mainly noted some spots where it appeared that perhaps there was a mix of tab vs space in the github diff view.
src/libnpfs/conn.c
Outdated
| n = snprintf (buf, | ||
| size, | ||
| "[%"PRIdMAX".%-3"PRIdMAX"] ", | ||
| (intmax_t)c.tv_sec, | ||
| (intmax_t)c.tv_usec/1000); |
There was a problem hiding this comment.
Check spacing here. Alignment is off in the diff view.
There was a problem hiding this comment.
Fixed. libnpfs uses linux kernel style indent (tabs only), so alignment is not expected. I was improperly mixing in spaces without thinking.
src/libnpfs/npstring.c
Outdated
| if (vsnprintf (s, len, fmt, ap) >= len) | ||
| strncpy (&s[len - 4], "...", 4); |
There was a problem hiding this comment.
Another alignment issue (tab vs space?)
src/libnpfs/fmt.c
Outdated
| res = np_deserialize_p9dirent (&qid, | ||
| &offset, | ||
| &type, | ||
| dname, |
There was a problem hiding this comment.
Check tab vs space here as well.
src/libnpfs/fmt.c
Outdated
| if (q->type == Qtfile) | ||
| buf[n++] = 'f'; |
There was a problem hiding this comment.
Just double checking that the test here shouldn't be (q->type & Qtfile) as in the previous test for Qtsymlink? (I guess this makes sense if f is a fallthrough test for regular files that are not other types, but just wanted to double check after noticing the difference)
There was a problem hiding this comment.
This is confusing. The value of Qtfile is zero so really part of this field is an enum and part is a bitfield. I restructured the function to make that more obvious and added a commit to fix a couple of other places where Qtfile's value was confusingly assumed to be zero.
Problem: protocol tracing truncates messages at 1024 bytes in np_logmsg(), but decoding dentries in Rreaddir will require a larger buffer. Allocate a 1 mbyte buffer in Npsrv and use it for trace formatting. Pass this buffer to the implementation Npsrv->logmsg() callback, bypassing np_logmsg().
Problem: spaces and tabs are used inconsistently in npstring.c. Use tabs for code indentation.
Problem: there is no indication in the output when the protocol trace buffer is truncated due to overflow. Print "..." in that case.
Problem: the Rreaddir protocol trace shows a (truncated) hex dump for the list of dentries, which is not super helpful. Decode the dentries.
Problem: nothing is printed in the qid type field for type Qtfile (0). If none of the other "base type" bits (Qtdir, Qtauth, Qtsymlink, Qtlink) are set, assume Qtfile and render that as 'f'. Also, Qtlink was missing so render it as 'l' and change Qtexcl from 'l' to 'x'.
Problem: initializion of qid.type from a 'struct stat' or 'struct dirent' is confusing. These functions set the qid.type "base type" to either Qtfile, Qtdir, or Qtsymlink. Just do that instead of relying on the fact that Qtfile has a value of zero and ORing in the other values.
garlick
force-pushed
the
improve_logging
branch
from
2f41ed7 to
2dc8a32
Compare
Merge Queue Status✅ The pull request has been merged at 2dc8a32 This pull request spent 6 seconds in the queue, with no time running CI. Required conditions to merge
|
2 participants
Problem: while working on #164, it became annoying that the full
Rreaddirresponse is not decoded.Fix that.
Now we see the dentry list, e.g.
where before the list of dirents was only a truncated hex dump