From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()
Date: Wed, 3 Apr 2024 16:48:36 -0400 [thread overview]
Message-ID: <20240403204836.GC1949464@coredump.intra.peff.net> (raw)
In-Reply-To: <b48fd3ee-2975-481f-aa0e-8ec4d07ea705@web.de>
On Wed, Apr 03, 2024 at 12:01:13PM +0200, René Scharfe wrote:
> > For mksnpath(), though, I
> > suspect that trying to format a very long name could result in the
> > output being full of uninitialized bytes.
> >
> > It only has one caller, which creates "foo~1" when we got EEXIST from
> > "foo". So I doubt you can get up to too much mischief with it. But it
> > could easily be replaced by mkpathdup() (or even a reusable strbuf
> > outside the loop if you really wanted to hyper-optimize)
> >
> > And then we could get rid of mksnpath() entirely, and its horrible
> > bad_path failure mode.
>
> mkpath() would be perfect but unusable in multiple threads. Cleaning
> up after mkpathdup() is a bit iffy in that caller. strbuf would be a
> bit much in that error path, I think, and you might have to export or
> reimplement strbuf_cleanup_path().
Yeah, I'd prefer not to go to mkpath(), even though that's the simplest
thing, just because we should be reducing the subtle non-reentrant parts
of the code over time. I don't think the cleanup handling for
mkpathdup() is too bad:
diff --git a/apply.c b/apply.c
index 432837a674..15dfe607ff 100644
--- a/apply.c
+++ b/apply.c
@@ -4502,20 +4502,25 @@ static int create_one_file(struct apply_state *state,
unsigned int nr = getpid();
for (;;) {
- char newpath[PATH_MAX];
- mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
+ char *newpath = mkpathdup("%s~%u", path, nr);
res = try_create_file(state, newpath, mode, buf, size);
- if (res < 0)
+ if (res < 0) {
+ free(newpath);
return -1;
+ }
if (!res) {
- if (!rename(newpath, path))
+ if (!rename(newpath, path)) {
+ free(newpath);
return 0;
+ }
unlink_or_warn(newpath);
+ free(newpath);
break;
}
if (errno != EEXIST)
break;
++nr;
+ free(newpath);
}
}
return error_errno(_("unable to write file '%s' mode %o"),
It even gets a little easier if you hoist it to a strbuf outside the
loop, as you don't need to free on each loop iteration. That seems worth
it to me to get rid of a function that IMHO is mis-designed (it is
really only useful if you assume paths are bounded by PATH_MAX, which we
know isn't always true).
> > diff --git a/usage.c b/usage.c
> > index 09f0ed509b..5baab98fa3 100644
> > --- a/usage.c
> > +++ b/usage.c
> > @@ -19,8 +19,10 @@ static void vreportf(const char *prefix, const char *err, va_list params)
> > }
> > memcpy(msg, prefix, prefix_len);
> > p = msg + prefix_len;
> > - if (vsnprintf(p, pend - p, err, params) < 0)
> > + if (vsnprintf(p, pend - p, err, params) < 0) {
> > + if (snprintf(p, pend - p, "could not format error: %s", err) < 0)
> > *p = '\0'; /* vsnprintf() failed, clip at prefix */
> > + }
> >
> > for (; p != pend - 1 && *p; p++) {
> > if (iscntrl(*p) && *p != '\t' && *p != '\n')
> >
> > Though most of the rest of the function is not that useful (it is mostly
> > removing unreadable chars, and hopefully we do not have any of those in
> > our format strings!).
>
> For warnings and usage messages this would keep the prefix and not
> die. This would look a bit strange, no?
>
> usage: could not format error: TERRIBLE MESSAGE!
Sure, but I think any solution here is going to look strange. Keep in
mind that we're trying to improve the case where we print _nothing_
useful at all. If you do see this on a non-fatal message, the subsequent
messages may be informative. E.g.:
error: could not format error: unable to open loose object %s
fatal: bad object HEAD~12
is probably better than just exiting after the first.
> Yes, showing errno doesn't add that much value. Except in this case it
> shows that there's something going on that I don't understand. Dare I
> dig deeper? Probably not..
Well, let us know if you do. ;)
-Peff
next prev parent reply other threads:[~2024-04-03 20:48 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-31 18:53 [PATCH] mem-pool: use st_add() in mem_pool_strvfmt() René Scharfe
2024-04-01 3:36 ` Jeff King
2024-04-02 13:48 ` René Scharfe
2024-04-03 1:18 ` Jeff King
2024-04-03 10:01 ` René Scharfe
2024-04-03 20:48 ` Jeff King [this message]
2024-04-03 21:20 ` Jeff King
2024-04-05 13:10 ` René Scharfe
2024-04-05 17:45 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240403204836.GC1949464@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=l.s.r@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).