Messages in this thread | | | Date | Thu, 14 Jul 2022 05:52:56 +0900 | From | Dominique Martinet <> | Subject | Re: [PATCH v5 10/11] net/9p: add p9_msg_buf_size() |
| |
Christian Schoenebeck wrote on Wed, Jul 13, 2022 at 03:06:01PM +0200: > > > + case P9_TWALK: > > > + BUG_ON(strcmp("ddT", fmt)); > > > + va_arg(ap, int32_t); > > > + va_arg(ap, int32_t); > > > + { > > > + uint i, nwname = max(va_arg(ap, int), 0); > > > > I was about to say that the max is useless as for loop would be cut > > short, but these are unsigned... So the code in protocol.c p9pdu_vwritef > > 'T' has a bug (int cast directly to uint16): do you want to fix it or > > shall I go ahead? > > I'd either send a separate patch today for fixing 'T', or if you want > to handle it by yourself, then just go ahead.
I'd appreciate if you have time, doesn't make much difference though
> > > + case P9_TCREATE: > > > + BUG_ON(strcmp("dsdb?s", fmt)); > > > + va_arg(ap, int32_t); > > > + { > > > + const char *name = va_arg(ap, const char *); > > > + if ((c->proto_version != p9_proto_2000u) && > > > + (c->proto_version != p9_proto_2000L)) > > > > (I don't think 9p2000.L can call TCREATE, but it doesn't really hurt > > either) > > Yes, Tcreate is only 9p2000 and 9p2000.u. Semantically this particular > check here means "if proto == 9p.2000". I can't remember anymore why I > came up with this inverted form here. I'll change it to "if > (c->proto_version == p9_proto_legacy)".
Sounds good.
> > > + case P9_TRENAMEAT: > > if we have trenameat we probably want trename, tunlinkat as well? > > What's your criteria for counting individually vs slapping 8k at it? > > > > In this particular case, oldname/newname are single component names > > within a directory so this is capped at 2*(4+256), that could easily fit > > in 4k without bothering. > > I have not taken the Linux kernel's current filename limit NAME_MAX > (255) as basis, in that case you would be right. Instead I looked up > what the maximum filename length among file systems in general was, > and saw that ReiserFS supports up to slightly below 4k? So I took 4k > as basis for the calculation used here, and the intention was to make > this code more future proof. Because revisiting this code later on > always takes quite some time and always has this certain potential to > miss out details.
hmm, that's pretty deeply engrained into the VFS but I guess it might change eventually, yes.
I don't mind as long as we're consistent (cf. unlink/mkdir below), in practice measuring doesn't cost much.
> Independent of the decision; additionally it might make sense to add > something like: > > #if NAME_MAX > 255 > # error p9_msg_buf_size() needs adjustments > #endif
That's probably an understatement but I don't mind either way, it doesn't hurt.
> > > + BUG_ON(strcmp("dsds", fmt)); > > > + va_arg(ap, int32_t); > > > + { > > > + const char *oldname = va_arg(ap, const char *); > > > + va_arg(ap, int32_t); > > > + { > > > + const char *newname = va_arg(ap, const char *); > > > > (style nitpick) I don't see the point of nesting another level of > > indentation here, it feels cleaner to declare oldname/newname at the > > start of the block and be done with it. > > Because va_arg(ap, int32_t); must remain between those two > declarations, and I think either the compiler or style check script > was barking at me. But I will recheck, if possible I will remove the > additional block scope here.
Yes, I think it'd need to look like this:
case foo: BUG_ON(...) va_arg(ap, int32_t); { const char *oldname = va_arg(ap, const char *); const char *newname; va_arg(ap, int32_t); newname = va_arg(ap, const_char *); ... } or { const char *oldname, *newname; oldname = va_arg(ap, const char *); va_arg(ap, int32_t) newname = va_arg(ap, const char *); ... } I guess the later is slightly easier on the eyes
> > > + /* small message types */ > > > > ditto: what's your criteria for 4k vs 8k? > > As above, 4k being the basis for directory entry names, plus PATH_MAX > (4k) as basis for maximum path length. > > However looking at it again, if NAME_MAX == 4k was assumed exactly, > then Tsymlink would have the potential to exceed 8k, as it has name[s] > and symtgt[s] plus the other fields.
yes.
> > > + case P9_TSTAT: > > this is just fid[4], so 4k is more than enough > > I guess that was a typo and should have been Twstat instead?
Ah, had missed this because 9p2000.L's version of stat[n] is fixed size. Sounds good.
> > > + case P9_RSTAT: > > also fixed size 4+4+8+8+8+8+8+8+4 -- fits in 4k. > > Rstat contains stat[n] which in turn contains variable-length string > fields (filename, owner name, group name)
Right, same mistake.
> > > > + case P9_TSYMLINK: > > that one has symlink target which can be arbitrarily long (filesystem > > specific, 4k is the usual limit for linux but some filesystem I don't > > know might handle more -- it might be worth going through the trouble of > > going through it. > > Like mentioned above, if exactly NAME_MAX == 4k was assumed, then > Tsymlink may even be >8k.
And all the other remarks are 'yes if we assume bigger NAME_MAX' -- I'm happy either way.
> > rest all looks ok to me. > > Thanks for the review! I know, that's really a dry patch to look > at. :)
Thanks for writing it in the first place ;)
-- Dominique
| |