Hi Bernhard,
On Sun, Apr 10, 2022 at 7:51 PM Bernhard Reutner-Fischer
<rep.dot....@gmail.com> wrote:
> > 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?
No, just unlink()ing is not enough, and yes, we really do gain
something by fsync()ing. We specially do things in the order of: read,
unlink, fsync, seed. If we crash at any time before "seed", no harm
done: we haven't seeded. If we crash after seeding, however, that file
_must_ be actually unlink()ed (with subsequent writes ordered against
that). It is necessary.
> 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?
Not a huge fan of that, but okay, sure. bb_simple_perror_msg should
suppress errno=0 "Success" messages anyway, so not the worst thing.
Saves 18 bytes.
>
> In v5, seed_from_file_if_exists i personally think that the errno
> handling does not improve readability TBH. But that's just me.
Also not a huge fan of removing that, but nonetheless I'll cut back
the errno handling entirely there. Should be functionally the same.
Saves an additional 18 bytes.
> But either way, if seed_len==0 why you don't unlink the file?
I think you misread the function. For seed_len==0 I do unlink the
file. But if I try to do that, and it fails, it's not an error, since
nothing is credited in that case.
> 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?
Filesystem corruption for battery powered devices, package managers
and templates that "touch" a file, etc. More common than you might
think. In that case, the right course of action is to unlink the file
and then act as though it didn't exist.
> 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)
> In main, i'd have used xopen_xwrite_close if a single error-code is
> enough or needed at all.
No, again, not acceptable, since the fd must be fsync()ed before
renaming it to become creditable.
> And we have rename_or_warn() if you can live without exit(64) in this
> case.
This doesn't reduce code size.
I'll send you a v6 that saves 249 more bytes of space, with a few more
tricks (like locking dfd instead of a separate lock file). I hope
you'll commit that.
Jason
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
