On Sun, 10 Apr 2022 17:39:50 +0200
"Jason A. Donenfeld" <ja...@zx2c4.com> wrote:
> Hi Bernhard,
>
> On Sun, Apr 10, 2022 at 5:29 PM Bernhard Reutner-Fischer
> <rep.dot....@gmail.com> wrote:
> > > > Also seed_dir et al are only used in main so i'd move them there and
> > > > not have them static. Doesn't make much difference though.
> > >
> > > seed_dir isn't only used from main. It doesn't impact binary size.
> >
> > I'd argue that it should only be used from main.
> > There's no benefit to have additional code dealing with the dir if what
> > you really want and operate on is the respective seed file, no?
> >
> > What is that fsync(dfd) supposed to achieve? I'd remove all this, i
> > don't really see why it's needed.
>
> When you tell me to remove things just on the basis of not
> understanding it, you greatly diminish your credibility in this
> discussion, maintainer or not. It's one thing to ask why it's needed.
> It's another thing to say remove it because you don't know why. So
> I'll pretend you only asked the question, but didn't also issue that
> removal request. :)
heh :)
>
> The reason the fsync(dfd) is there is so that the file is removed from
> the file system after we read the file, but importantly before we
> seed, so that the same seed is never credited twice. Removing
> fsync()ing, like removing flock()ing is highly problematic. I'm not
> going to remove that.
I'd be surprised if it wouldn't be enough to just unlink.
Unlink is supposed to only return after the link has been removed.
So i wonder why you think you need the extra fsync()?
You can as well crash or lose power while the fsync still running.
Do you really gain all that much by that fsync?
>
> > > Sometimes it's tmpfs, sometimes it isn't. And sometimes programs get
> > > oom'd mid-execution and can't cleanup files. O_EXCL is not the right
> > > way of doing this.
> >
> > Yea, so if this is supposed to be run even after the initial invocation
> > at boot, i.e. anytime via timers, then yes.
> > The lockfile is conceptually not much different than a pidfile i
> > suppose. But there is probably no benefit in creating a pidfile and
> > locking that as opposed to what you manually do now. Hmm.
>
> Pids are racey. A lock file is a lock on the exact resource we want.
Yes. What i mean was that it does not matter much _what_ file you lock.
>
> > Btw, you seem to be touching errno a lot and you do that in addition to
> > ret.
> > Maybe this can be simplified a little bit?
>
> In v4 I went back to making the helpers act like libc functions, so I
> could use perror, resulting in less code size. And I checked in IDA
> that this indeed does result in less code size. The reason is that all
> those helpers except one get inlined by the compiler, and then the
> errno handling elides. This also has the benefit of making the code
> easier to read, because there's a consistent error path.
>
> Anyway, have a look at v4. I think based on the above, it's mergeable?
It looks quite ok already. It would be nice if we could further reduce
the size though.
For example, in read_new_seed() we probably don't need the errno=EIO,
for the user it should be sufficient to
bb_simple_error_msg("unable to read new seed"); i suppose?
In v5, seed_from_file_if_exists i personally think that the errno
handling does not improve readability TBH. But that's just me.
But either way, if seed_len==0 why you don't unlink the file?
In this case the file would be garbage anyway, no? Or, the other way
round:
Why would the file exist and be readable but have no data, IIUC?
What the function essentially does is
errno = 0;
seed_len = open_read_close(filename, seed, sizeof(seed));
if (seed_len > 0) {
if (unlink(filename) != 0)
bb_perror_msg_and_die("not seeding, unable to remove");
sha256_hash;sha256_hash;
if (seed_rng(seed, seed_len, credit) == 0)
printf("Seeding %zd bits etc"), return 0;
}
bb_simple_error_msg("not seeding");
return 1; // failed
Or, if you insist,
/* absolutely try to get the metadata on disk for extra paranoia */
if (unlink(filename) != 0 || fsync(dtb) != 0)
In main, i'd have used xopen_xwrite_close if a single error-code is
enough or needed at all.
And we have rename_or_warn() if you can live without exit(64) in this
case.
HTH,
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
