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.
Details
Details
- Reviewers
- None
- Commits
- rG4cd4841a2773: Modularize uuencode and uudecode by wrapping them in bintrans.c
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Hi, thanks for this review!
| usr.bin/uuencode/bin2text2bin.c | ||
|---|---|---|
| 76 | 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 | ||
| 70 | 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. | |
Comment Actions
Remove uudecode remnants from Makefile.inc1, etc/mtree/BSD.tests.dist, targets/pseudo/tests/Makefile.depend, and targets/pseudo/userland/Makefile.depend
Comment Actions
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 | ||
|---|---|---|
| 46 ↗ | (On Diff #105058) | Maybe use getprogname() here? Like the suggested edit |
| 52 ↗ | (On Diff #105058) | The basename() were somewhat confusing to me, was e.g. bintrans ./uuencode a valid usage? If not, the basename() call should be removed here. |
| 68 ↗ | (On Diff #105058) | OPTIONAL: It looks like we should be using EX_USAGE here? |
| 70 ↗ | (On Diff #105058) | 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 */. |
Comment Actions
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 | ||
|---|---|---|
| 70 ↗ | (On Diff #105058) | 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. |