Page MenuHomeFreeBSD

Modularize uuencode and uudecode by wrapping them in bintrans.c
ClosedPublic

Authored by pstef on Nov 11 2021, 5:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 11:37 PM
Unknown Object (File)
Tue, Nov 26, 11:37 PM
Unknown Object (File)
Tue, Nov 26, 11:37 PM
Unknown Object (File)
Tue, Nov 26, 11:37 PM
Unknown Object (File)
Tue, Nov 26, 11:37 PM
Unknown Object (File)
Mon, Nov 18, 9:55 AM
Unknown Object (File)
Mon, Nov 18, 9:28 AM
Unknown Object (File)
Mon, Nov 18, 9:27 AM

Details

Summary
The program will be installed as bintrans, uuencode, uudecode,
b64encode, and b64decode and will be responsible for running the coders
according to their historical behavior.

Additionally, bintrans will be able to take a parameter designating
the coder and accept all its options in this form:
bintrans <coder> [options]
and the behavior should be the same as if
<coder> [options]
was invoked.
This has the advantage that adding coders won't require installing them
as binaries.

Move uudecode files to uuencode since the latter is the one that
provides the manual page.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

pstef requested review of this revision.Nov 11 2021, 5:21 PM
pstef created this revision.
delphij added inline comments.
usr.bin/uuencode/bin2text2bin.c
76 ↗(On Diff #104240)

I think this should be defined as a global constant as the mapping can also be used to translate the codec enum back to their names.

usr.bin/uuencode/uudecode.c
71

You might want to have a uudecode.h for all these functions...

Hi, thanks for this review!

usr.bin/uuencode/bin2text2bin.c
76 ↗(On Diff #104240)

I don't disagree, but it wouldn't serve a purpose yet. For now I decided to limit the scope of its visibility.

usr.bin/uuencode/uudecode.c
71

There is one in this patch and there will be another one coming from D32945. I'm not sure it's worth extracting to a separate header file yet.

Remove uudecode remnants from Makefile.inc1, etc/mtree/BSD.tests.dist, targets/pseudo/tests/Makefile.depend, and targets/pseudo/userland/Makefile.depend

pstef retitled this revision from Conflate uudecode and uuencode to Modularize uuencode and uudecode by wrapping them in bintrans.c.
pstef edited the summary of this revision. (Show Details)

Rename bin2text2bin to bintrans. Update the manual page.

Personally I don't really like the bintrans name as it's colliding with some tools that I am using at work, but it's probably just me and I don't insist that :)

Please see a few minor nits in inline comments.

usr.bin/uuencode/bintrans.c
47

Maybe use getprogname() here? Like the suggested edit

53

The basename() were somewhat confusing to me, was e.g. bintrans ./uuencode a valid usage? If not, the basename() call should be removed here.

69

OPTIONAL: It looks like we should be using EX_USAGE here?

71

OPTIONAL: It would be nice if both main_encode and main_decode were marked as __dead (because they don't return), or add an exit(EX_OK) with a comment like /* NOTREACHED */.

Address some of the comments made by delphij.

Thanks again for reviewing this!

Personally I don't really like the bintrans name as it's colliding with some tools that I am using at work, but it's probably just me and I don't insist that :)

We talked about it on IRC recently and bintrans was considered an improvement. In the meantime I learned about metamail and mmencode/mimencode which sound more reasonable than bintrans, but are already taken.

I think bintrans is not that bad; the program does what it says on the tin: translate or transform binary data. I've updated the topic on -arch so maybe someone comes up with a much better name, then I'll rename this program again.

usr.bin/uuencode/bintrans.c
71

This is the only of your suggestions that I wasn't sure how to incorporate. I've already introduced the issue of duplicating these declarations which means if any of them needs to be updated, they all have to be. Adding __dead2 would exacerbate this problem. On the other hand, I don't think they will ever change, but I might be wrong.

Moving these to their own .h files would be closer to the standard practice, but on the other hand there are so few of these declarations that that would look silly.

I could move them all into one bintrans.h file and include from bintrans.c and each of the files defining the functions. But that has the flaw that some declarations would unnecessarily become visible in some translation units.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 18 2022, 8:55 AM
This revision was automatically updated to reflect the committed changes.