Messages in this thread | | | Date | Mon, 25 Apr 2022 00:19:09 -0400 | From | Kent Overstreet <> | Subject | Re: [PATCH v2 1/8] lib/printbuf: New data structure for heap-allocated strings |
| |
On Mon, Apr 25, 2022 at 03:44:34AM +0100, Matthew Wilcox wrote: > On Sun, Apr 24, 2022 at 04:46:03PM -0700, Joe Perches wrote: > > > + * pr_human_readable_u64, pr_human_readable_s64: Print an integer with human > > > + * readable units. > > > > Why not extend vsprintf for this using something like %pH[8|16|32|64] > > or %pH[c|s|l|ll|uc|us|ul|ull] ? > > The %pX extension we have is _cute_, but ultimately a bad idea. It > centralises all kinds of unrelated things in vsprintf.c, eg bdev_name() > and clock() and ip_addr_string().
And it's not remotely discoverable. I didn't realize we had bdev_name() available as a format string until just now or I would've been using it!
> Really, it's working around that we don't have something like Java's > StringBuffer (which I see both seq_buf and printbuf as attempting to > be). So we have this primitive format string hack instead of exposing > methods like: > > void dentry_string(struct strbuf *, struct dentry *);
Exactly!
> as an example, > if (unlikely(ino == dir->i_ino)) { > EXT4_ERROR_INODE(dir, "'%pd' linked to parent dir", > dentry); > return ERR_PTR(-EFSCORRUPTED); > } > > would become something like: > > if (unlikely(ino == dir->i_ino)) { > struct strbuf strbuf; > strbuf_char(strbuf, '\''); > dentry_string(strbuf, dentry); > strbuf_string(strbuf, "' linked to parent dir"); > EXT4_ERROR_INODE(dir, strbuf); > return ERR_PTR(-EFSCORRUPTED); > } > > which isn't terribly nice, but C has sucky syntax for string > construction. Other languages have done this better, including Rust.
Over IRC just now you proposed "%p(%p)", dentry_name, dentry - I'm _really_ liking this idea, especially if we can get glibc to take it.
Then your ext4 example becomes just
if (unlikely(ino == dir->i_ino)) { EXT4_ERROR_INODE(dir, "'%p(%p)' linked to parent dir", dentry_name, dentry); return ERR_PTR(-EFSCORRUPTED); }
And you can cscope to the pretty-printer! And dentry_name becomes just
void dentry_name(struct printbuf *out, struct dentry *dentry) { ... }
Which is quite a bit simpler than the current definition.
Sweeeeeet.
| |