Page MenuHomeFreeBSD

Implement base64(1)
ClosedPublic

Authored by pstef on Nov 11 2021, 5:27 PM.

Details

Summary

To be more precise, emulate GNU base64 using b64encode and b64decode.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pstef requested review of this revision.Nov 11 2021, 5:27 PM
pstef created this revision.
pstef retitled this revision from Implement base64 to Implement base64(1).Nov 11 2021, 5:38 PM

Mostly implement long options for compatibility.

Looks mostly good to me, could you please take a look at the minor nits in the inline comments?

usr.bin/uuencode/bin2text2bin.c
110 ↗(On Diff #104242)

Could you please replace magic numbers here (-2 and -3 below) with symbolic names instead? This would help readers to better understanding their meanings.

Usually this is done by defining an enum like:

enum base64_options
{
    OPTION_BASE64_HELP = -2;
    OPTION_BASE64_VERSION = -3;
};

(Personally I'd probably just use 1 and 2 here)

116 ↗(On Diff #104242)

Please replace this block with a switch/case block.

121 ↗(On Diff #104242)

Please extract this into a static function instead of implementing it here.

It would be nice if we can do something more descriptive (e.g. FreeBSD base64(1) or even FreeBSD base64 version X.XXinstead of just FreeBSD as the latter might be surprising for users) by the way.

124 ↗(On Diff #104242)

Please extract this to a static function e.g. usage_base64().

usr.bin/uuencode/uudecode.c
85

style: there should be a new line after { if there is no local variables.

usr.bin/uuencode/uuencode.c
82

Style: add a blank line between { and raw.

Hi, thanks for this review!

usr.bin/uuencode/bin2text2bin.c
110 ↗(On Diff #104242)

I'll think about what I can do here. The cryptic values were intentional, to limit the number of places you have to update when adding a new option. With getopt_long() it's usually 3: enums, optstring and longopts. It's less with getopt_long_only() since you can omit optstring, and short options work as they're recognized as a prefix of the long option - but that's asking for trouble.

116 ↗(On Diff #104242)

In some incarnations of this loop, I checked for ch < -1 or similar. If I make this a switch and decide to revisit that idea later, I would have to convert the switch again.

121 ↗(On Diff #104242)

I thought making this a function wasn't worth the effort as it would only have 1 place calling it.

I didn't want to write anything more because I would have to set a new tradition of version numbering of this program that never had it before. The option only exists so that using it is recognized and doesn't invoke an error.

124 ↗(On Diff #104242)

I can do that, but is it worth it, though? Only one place would call it.

usr.bin/uuencode/uuencode.c
82

Please correct me if I'm wrong, but I believe that empty lines in cases like this one are optional, especially after 208be1617cda8ad375aa4de8e34cee10775662b4.

Address delphij's comments.

This revision is now accepted and ready to land.Apr 16 2022, 5:42 PM