On 13.11.22 15:06, Omar Polo wrote:
On 2022/11/13 13:34:52 +0100, Alexander Klimov <grandmas...@al2klimov.de> wrote:
On 13.11.22 12:36, Stuart Henderson wrote:
One minor issue, argp-standalone only provides a static library, so
this should use BUILD_DEPENDS rather than LIB_DEPENDS (essentially
LIB_DEPENDS is invalid unless there is a WANTLIB entry to go with it).
+1
Fixed. (See https://github.com/Al2Klimov/f3/commits/openbsd-port .)
I would like guidance from someone who knows pledge better than me
about the NULL vs "" for execpromises. The manual doesn't mention it
but AFAIK we are not using it at all and I think it would be unwise
Well...
grep -rnIFe 'pledge("' /usr/ports |grep -vFwe NULL -e exec
says:
can't speak for these
/usr/ports/devel/woboq_codebrowser/patches/patch-generator_main_cpp:25:+
if (pledge("stdio rpath wpath cpath tty", "") == -1) {
/usr/ports/devel/woboq_codebrowser/patches/patch-indexgenerator_indexer_cpp:24:+
if (pledge("stdio rpath wpath cpath tty", "") == -1) {
but these are like that because upstream decided to use "" for
execpromises. When I fixed the pledge promise there I kept upstream
style.
/usr/ports/mail/mblaze/patches/patch-mdeliver_c:11:- xpledge("stdio
rpath wpath cpath", "");
/usr/ports/mail/mblaze/patches/patch-mdeliver_c:12:+ xpledge("stdio
rpath wpath cpath fattr", "");
/usr/ports/mail/mblaze/patches/patch-mdeliver_c:15:+ xpledge("stdio
rpath wpath cpath", "");
...
to do something different here.
the second argument to pledge applies only to the child processes. I
haven't read f3 code, but assuming it works (not tested) since it
lacks pledge "proc" I guess it's practically the same if execpromise
is NULL or "". fwiw I prefer NULL tho.
Adjusted.
I think it was already mentioned in this thread, but i wouldn't
unveil(NULL, NULL) since you call pledge next line without the
"unveil" promise. Also, I'd prefer if it used something more usual
Adjusted.
for pledge instead of assert. In base, you'd find
if (pledge("...", NULL) == -1)
err(1, "pledge");
which is a better idiom that assert(!pledge("...", NULL)) IMHO. (not
counting that assert can be dearmed with -DNDEBUG.)
Fixed.
Regarding patches/patch-utils_c:
15 + if (chdir(*dev_path)) {
16 + err(errno, "Can't change working directory to %s at
%s()
16 ", *dev_path, __func__);
17 + }
I don't know how useful it is to terminate the process with an exit
status of `errno'. I'd just err(1, "...");
Also, I'm not sure how I feel about trying to chroot and not reporting
the error if errno is EPERM. Probably better to check with geteuid()?
But still meh.
Everything in patches/patch-utils_* is borrowed from upstream master.
patches/patch-utils_* will be obsolete with, say, v9.0.
While pledge and unveil have some magic in the kernel so that some
kind of libc operations always works (reading the timezone data for
example) when using chroot you may need to initialize some things
explicitly. I haven't read the code, have you double checked? if in
doubt, I'd just drop the chroot patch from the port and send it
upstream. Other operating system could benefit from having this
chrooted, and upstream developer should have a clearer idea of when/if
to chroot.
The way the missing posix_fadvise is handled i guess it's fine.
Looking at the linux manpage it says it's to "announce an intention to
access file data in a specific pattern in the future, thus allowing
the kernel to perform appropriate optimizations" so neutering it
should be ok.
Just one more comment regarding the Makefile patch: I'd add a MANDIR
variable set to ${PREFIX}/share/man/ and overwrite it in the port'
makefile
MAKE_ENV += MANDIR=${PREFIX}/man/
I think I understand and would be able to do this, but the author
doesn't like TOO OSspecific stuff in upstream (such as unveil(2),
pledge(2)) and this neither improves anything significantly nor adds
support for any OS. Let's think twice about this and maybe look for
something else to also adjust not to send more patches than necessary.
this way such patch could be easily upstreamed and removed from the
port in a future update.
Cheers,
Omar Polo
