On Tue 10-04-18 11:37:54, Jeff Mahoney wrote: > On 4/10/18 11:34 AM, Jan Kara wrote: > > On Tue 10-04-18 10:44:43, Jeff Mahoney wrote: > >> On 4/10/18 10:25 AM, Jan Kara wrote: > >>> On Mon 09-04-18 14:16:02, jeffm@xxxxxxxx wrote: > >>>> From: Jeff Mahoney <jeffm@xxxxxxxx> > >>>> > >>>> reiserfs_snprintf operates by handling the reiserfs-specific specifiers > >>>> itself and passing everything else to the generic vsnprintf routine. > >>>> That makes some assumptions about what the state of a va_list passed > >>>> by value to another function will be after the function returns. The > >>>> spec states that the contents of the va_list are undefined in that > >>>> case. It's worked so far but really only by the grace of gcc's internal > >>>> va_list implementation details. > >>>> > >>>> This patch reworks reiserfs_snprintf to process one % directive at > >>>> a time. Since directives can consume more than one argument when field > >>>> width and precision * operators are used, that means we need to interpret > >>>> the format string more than we used to do. (The kernel vsnprintf > >>>> specifically doesn't handle %n). Otherwise, we can use an unsigned long > >>>> to hold the argument and let the generic snprintf do the type handling. > >>>> The only exception is long long arguments on platforms where long long > >>>> is larger than long, which need special handling. > >>> > >>> So I wonder why is this correct for architectures where sizeof(int) != > >>> sizeof(long). If you do va_arg(va, long) for an argument which is actually > >>> int, you'll consume more bytes from the stack than you should. Or am I > >>> missing some va_arg() subtlety? > >> > >> It's possible I'm incorrect here. I'm no expert on how variadics are > >> constructed across architectures. My underlying assumption is that even > >> if the arguments are passed on the stack, they're not the same as stack > >> variables. There's no ABI between caller and callee, so I expect that > >> the stack slot would be the size of a register, which is long. So the > >> int returned by va_arg would only take up e.g. 4 bytes on the stack but > >> the space va_arg uses would take e.g. 8 bytes. Yes, it's fragile. > > > > Well, another equally sensible implementation could be that the compiler > > just arranges for storing all arguments on stack using their native size by > > the caller (who knows all the types) and va_start + va_arg know how to > > consume these arguments from the stack. And with such implementation you'd > > need to make sure you pass a proper type to va_arg()... > > I suppose. I'd like to avoid doing that entirely. I'll give the > non-va_copy pointer() implementation a try. OK. > >>>> The result is a va_list that is consistent within reiserfs_snprintf > >>>> and isn't passed by value anymore. > >>>> > >>>> This isn't an ideal solution, but I didn't feel that teaching pointer() > >>>> about a %pV variant that didn't use va_copy would get much traction for > >>>> a number of reasons. > >>> > >>> Frankly, I don't like the format string parsing duplication in your patch > >>> and I'm afraid it will be a maintenance issue. Can't we have a variant > >>> of vsnprintf() that would take a pointer to va_list (like you use in > >>> your patch for some functions) so that its contents is well defined after > >>> the function returns? > >> > >> That would certainly solve the problem, but I didn't go that route > >> because the interface would end up being pretty fragile. For any case > >> other than consuming a single (potentially compound) format unit, it'd > >> be pretty easy to get out of sync. > > > > How come? You'd pass the format string containing only non-reiserfs > > specifiers and va_list * to new_vsnprintf(). It would consume as many args > > as it should. Then you process next reiserfs specifier consuming some arg, > > etc. I don't see what can get out of sync there... > > I meant more as a general API. I have code for reiserfs ready to go to > do this, though. Ah, OK, I see. Yes, it's not a great API but it gets the work done... Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
