Wireshark-bugs: [Wireshark-bugs] [Bug 8787] 9P dissector - compute fids on first visit
Comment # 13
on bug 8787
from Dominique Martinet
Well, to be fair, "tmp" was a variable I carried on from the original code. . .
Although given how much I've rewritten, I guess it's completely my fault if
it's still there.
Anyway, I started refactoring the code, and while I'm not always happy with
some aesthetics most of it isn't going too badly...
It actually made me find an unhandled error reply (and a few minor typos I
probably wouldn't have noticed otherwise!), and I'm not completely sure how to
fix that one as its action isn't to simply invalidate a fid but to give it back
its previous value. (basically, create will create and open a file, the fid
given points to the directory where the file is created and is changed to the
created file on success, but stays valid for the directory on error...)
Possible solutions that spring to mind:
- Don't set it in the request, but in the reply. This makes the error handling
unneeded, but it means to allocate one more string in the request and get it
back in the reply/free it there - if stuff works and we get either an error or
expected reply it's ok, but it's otherwise leaked till the end of the capture.
- Set in in the request as it's currently done, but on error either remove the
new fid from the tree (doesn't look possible at all in se_trees, which kinda
makes sense given there's no single-value se_free either), or somehow find the
previous value and insert it again (that'll require to keep the packet number
of the request to get this back, but it's better than keeping a whole string
imo, only drawback is one more insertion in the tree of an item we do not even
need to alloc)
I'd probably prefer the later solution at the moment, but any insight is
welcome.
(<kid>BTW, this would only have affected the firstpass logic part :P</kid>)
Another detail/request for preference: on TWALK, there's a loop going through
multiple path components, and each component is both printed and appended to an
ephemeral string to add fid path on first pass. I personally prefer looping
twice and separate both logics here as well, but I guess you'd have a "if
(firstpass)" for each iteration? (I'm not overly worried about performance
here, most walks only have one element anyway...)
Anyway, have fun on Sharkfest! :)
You are receiving this mail because:
- You are watching all bug changes.
