Re: [PATCH 5/6] pass st.st_size as hint for strbuf_readlink()
On Wed, Jul 25, 2018 at 08:41:00PM +0200, Torsten Bögershausen wrote: > On Tue, Jul 24, 2018 at 06:51:39AM -0400, Jeff King wrote: > > When we initially added the strbuf_readlink() function in > > b11b7e13f4 (Add generic 'strbuf_readlink()' helper function, > > 2008-12-17), the point was that we generally have a _guess_ > > as to the correct size based on the stat information, but we > > can't necessarily trust it. > > > > Over the years, a few callers have grown up that simply pass > > in 0, even though they have the stat information. Let's have > > them pass in their hint for consistency (and in theory > > efficiency, since it may avoid an extra resize/syscall loop, > > but neither location is probably performance critical). > > > > Note that st.st_size is actually an off_t, so in theory we > > need xsize_t() here. But none of the other callsites use it, > > and since this is just a hint, it doesn't matter either way > > (if we wrap we'll simply start with a too-small hint and > > then eventually complain when we cannot allocate the > > memory). > > Thanks a lot for the series. > > For the last paragraph I would actually vote the other way around - > how about something like this ? > Note that st.st_size is actually an off_t, so we should use > xsize_t() here. In pratise we don't expect links to be large like that, > but let's give a good example in the source code and use xsize_t() > whenever an off_t is converted into size_t. > This will make live easier whenever someones diggs into 32/64 bit > "conversion safetyness" I actually don't mind using xsize_t(), but if we're going into I think we should do it consistently. I.e., as a patch on top with that explanation. -Peff
Re: [PATCH 5/6] pass st.st_size as hint for strbuf_readlink()
On Tue, Jul 24, 2018 at 06:51:39AM -0400, Jeff King wrote: > When we initially added the strbuf_readlink() function in > b11b7e13f4 (Add generic 'strbuf_readlink()' helper function, > 2008-12-17), the point was that we generally have a _guess_ > as to the correct size based on the stat information, but we > can't necessarily trust it. > > Over the years, a few callers have grown up that simply pass > in 0, even though they have the stat information. Let's have > them pass in their hint for consistency (and in theory > efficiency, since it may avoid an extra resize/syscall loop, > but neither location is probably performance critical). > > Note that st.st_size is actually an off_t, so in theory we > need xsize_t() here. But none of the other callsites use it, > and since this is just a hint, it doesn't matter either way > (if we wrap we'll simply start with a too-small hint and > then eventually complain when we cannot allocate the > memory). Thanks a lot for the series. For the last paragraph I would actually vote the other way around - how about something like this ? Note that st.st_size is actually an off_t, so we should use xsize_t() here. In pratise we don't expect links to be large like that, but let's give a good example in the source code and use xsize_t() whenever an off_t is converted into size_t. This will make live easier whenever someones diggs into 32/64 bit "conversion safetyness"
[PATCH 5/6] pass st.st_size as hint for strbuf_readlink()
When we initially added the strbuf_readlink() function in
b11b7e13f4 (Add generic 'strbuf_readlink()' helper function,
2008-12-17), the point was that we generally have a _guess_
as to the correct size based on the stat information, but we
can't necessarily trust it.
Over the years, a few callers have grown up that simply pass
in 0, even though they have the stat information. Let's have
them pass in their hint for consistency (and in theory
efficiency, since it may avoid an extra resize/syscall loop,
but neither location is probably performance critical).
Note that st.st_size is actually an off_t, so in theory we
need xsize_t() here. But none of the other callsites use it,
and since this is just a hint, it doesn't matter either way
(if we wrap we'll simply start with a too-small hint and
then eventually complain when we cannot allocate the
memory).
Signed-off-by: Jeff King
---
builtin/init-db.c| 3 ++-
refs/files-backend.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 4ecf909368..12ddda7e7b 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -73,7 +73,8 @@ static void copy_templates_1(struct strbuf *path, struct
strbuf *template_path,
continue;
else if (S_ISLNK(st_template.st_mode)) {
struct strbuf lnk = STRBUF_INIT;
- if (strbuf_readlink(, template_path->buf, 0) < 0)
+ if (strbuf_readlink(, template_path->buf,
+ st_template.st_size) < 0)
die_errno(_("cannot readlink '%s'"),
template_path->buf);
if (symlink(lnk.buf, path->buf))
die_errno(_("cannot symlink '%s' '%s'"),
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a9a066dcfb..c110c2520c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -363,7 +363,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
/* Follow "normalized" - ie "refs/.." symlinks by hand */
if (S_ISLNK(st.st_mode)) {
strbuf_reset(_contents);
- if (strbuf_readlink(_contents, path, 0) < 0) {
+ if (strbuf_readlink(_contents, path, st.st_size) < 0) {
if (errno == ENOENT || errno == EINVAL)
/* inconsistent with lstat; retry */
goto stat_ref;
--
2.18.0.542.g2bf2fc4f7e
