xinstall: fix invocation of llvm-strip

[mhorne]

Summary

When executing strip(1), '--' is passed as an argument to explicitly
terminate the getopt(3) loop. The option parsing in llvm-strip doesn't
support this however, so setting XSTRIPBIN=llvm-strip results in an
unsupported argument error. llvm-strip(1) is otherwise
commandline-compatible with our strip(1), so just use the documented
argument format that is common to both.

[arichardson]

Surprised I haven't seen this, pretty sure I've built with XSTRIPBIN=llvm-strip before.

[arichardson]

Looking at git blame it seems like the "--" was added in D25551, so I probably tested before that was committed.

[mhorne]

In D30614#687694, @arichardson wrote:

Surprised I haven't seen this, pretty sure I've built with XSTRIPBIN=llvm-strip before.

I should clarify that this doesn't cause the build to fail, only that the warnings are quickly printed and the binaries will be left un-stripped. It becomes more obvious when running make -s XSTRIPBIN=llvm-strip buildworld.

[jilles]

The -- was added so that source pathnames starting with - would not break. I guess destination pathnames starting with - already did not work before D25551, so that was not a regression. This could be fixed differently by prepending ./ if the name starts with -.

[mhorne]

Handle from_name beginning with '-'.

[jilles]

usr.bin/xinstall/xinstall.c

1336–1338

The extra semicolon means it will return here always, and never run strip with the modified path.

Also, snprintf() will not fail if the buffer is too small. Instead, it will write a truncated string and return the full length.

An alternative to a fixed-size buffer could be heap allocation using asprintf(); this is not limited to a fixed length, but the resulting string needs to be freed afterward.

[eugen_grosbein.net]

I agree that snprintf() should be avoided here, as well as stack abuse. Please consider using asprintf() that allocated memory on heap all by itself dealing with space requirements automatically, just free the pointer just after posix_spawnp().

[mhorne]

Use asprintf() to allocate _from_name.

[mhorne]

Thank you both for the review. I was rushing a bit with the previous revision, and it shows. I've tested the success case for installing and stripping a file beginning with '-'. I'll look into adding a test for this case to install_test.sh.

[jilles]

usr.bin/xinstall/xinstall.c

1318

Perhaps name this variable something like prefixed_from_name.

1352

Calling free(NULL) need not be prevented.

[mhorne]

Address jilles' comments.

[mhorne]

usr.bin/xinstall/xinstall.c

1336

I'll note that this now goes slightly over 80 columns, but I found a linebreak here to be less legible.

[mhorne]

Any remaining concerns with this change?