Date: Wed, 26 Aug 2020 13:13:24 -0400 From: J David <j.david.lists@gmail.com> To: Konstantin Belousov <kib@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: pidfile_open() usage in "mount" Message-ID: <CABXB=RQJVddCs%2B_UhBBNm0NAxR8fvuU-bK-DaMYqD1Jv%2BvDwxg@mail.gmail.com> In-Reply-To: <20200826144013.GV2551@kib.kiev.ua> References: <CABXB=RRM7YusQL1gGVGD4s9xi9AB4F5AR5-Jqbp4G1WTZWWA%2BQ@mail.gmail.com> <20200826144013.GV2551@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Aug 26, 2020 at 10:40 AM Konstantin Belousov <kib@freebsd.org> wrote: > It is possible that both pidfile_open() and the new function would share > some significant amount of code. Not as much as one might hope, it looks like. There's already a pidfile_read() function that isn't exported, so that helps. And it looks like it might be worthwhile to pull out some of the path-handling code to a common static function. But if for no other reason than the O_WRONLY|O_TRUNC flags, the code necessarily diverges pretty hard after the flopen/flopenat (depending on version). Would the right approach to this look something like: - New static function: int pidfile_real_open( const char *pathp, mode_t ) that opens the right file, using the code & logic from lines existing pidfile_open() (pidfile.c lines 105 - 147 in HEAD). - Modify pidfile_open() to use pidfile_real_open() - New public function: int pidfile_get( const char *pathp, pid_t *pidptr, int ndelay ) returns 0 on success, EWOULDBLOCK (if ndelay is nonzero and lock fails) or ENOENT (if pidfile does not exist). Basically a wrapper around pidfile_open_int() and pidfile_read(). Notes: - pidfile_real_open: don't love the name; not sure if there is a naming convention for static functions that are called by public ones. - pidfile_real_open: different versions will be need for 12/HEAD and pre-12, due to lack of flopenat() in earlier releases. - pidfile_get: returns error value directly as I think the general move is away from returning -1 and setting errno in libraries like this - pidfile_get: ndelay would basically control the decision whether to skip (if nonzero) the nanosleep loop structure around pidfile_read, as happens in pidfile_open(). Probably not needed in most cases, but I've been bitten by unskippable sleeps before in the weirdest places, so why not? - pidfile_get: Would rather call this pidfile_read but that's taken. Since it's static though, could rename pidfile_read to pidfile_real_read (or whatever else is conventional, same as pidfile_real_open) and call this pidfile_read. That's my preference, but it's hard for me to tell what's actually better and what just looks better to me. If this all sounds OK, it shouldn't take me too long to come up with a patch and corresponding test(s). Thanks!
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CABXB=RQJVddCs%2B_UhBBNm0NAxR8fvuU-bK-DaMYqD1Jv%2BvDwxg>