Re: Cleaning up elkscmd and adding help text - Questions

Nils Stec
2015-03-27T17:59:28Z

Wow i really belive that writing this took some time... Thank you, this
gave me a lot of hints, do's and don'ts.

I read The Art of Unix Programming, but you showed me that i forgot a lot ;)

I think I'm going to rewrite my patches with all that things in mind,
will send them soon.

I hope my submissions will be ready very soon.

Nils

On 03/27/2015 02:19 PM, Jody Bruchon wrote:
> On 3/27/2015 4:19 AM, Nils Stec wrote:
>> I wrote a patch for cat which shows the user some usage-information.
>> Jody Bruchon used write() for this purpose, other tools are using
>> fprintf(stderr, ....).
>> I don't know which i should prefer. In my opinion i would rather use
>> fprintf.
>> STDERR should be the output for operations, right?
>
> Some of the programs already used write() and write() avoids dragging
> the code in for the printf family of functions (all ELKS programs are
> statically linked), so if there is NO usage of the printf family of
> functions in the program, it may make sense to use write() to minimize
> total program size. That being said, even a trivial usage message
> should mention the program being run in the usage, and it's not a good
> idea to assume that i.e. 'ls' is ALWAYS run as 'ls' since someone
> could symlink or alias it in the future. Because of this,
>
> fprintf(stderr, "usage: %s args\n", argv[0]);
>
> is a thousand times better than:
>
> write(STDERR_FILENO, "usage: ", 7);
> write(STDERR_FILENO, argv[0], strlen(argv[0]));
> write(STDERR_FILENO, "\n", 1);
>
> This results in three calls to write() and a call to strlen() when we
> could have just used one fprintf() to do all the work. In the future,
> I think many calls to write() will be eliminated, since even one call
> to a printf family function brings in the core code for all printf
> functions anyway.
>
>> Is this the right way to write code for this project? It's the first
>> time I'm doing this in here and i don't wan't to do things wrong...
>
> The cat patch should use fprintf(stderr, ...) since I've already
> brought in the printf function family with my error_read: code and
> write() is bad for the reasons outlined above.
>
> A few other comments:
>
> +#define NOT_OVERWRITING 0xABCD
> ...snip...
> + return NOT_OVERWRITING;
>
> If the user asks you to not overwrite (clobber), you should silently
> not clobber files and not consider it an error. If any program
> succeeds in doing what the user asked for, it should return success
> (0) with no messages. If you haven't read Eric S. Raymond's rules of
> the Unix philosophy, take a look at them; I find them to be very
> helpful guidelines when making program design decisions.
>
> + if (stat(srcname, &statbuf1) < 0) { // src nonexistent
>
> You can't write comments using // unless you're using a C99 compiler.
> bcc is a K&R C compiler that is not even guaranteed to be C89
> compliant, so no // comments. Use /* comment */ instead. Source
> comments are nice to have as long as they're not excessive or
> explaining something that is very obvious.
>
> +static int option_no_overwrite;
> +static int option_verbose;
> +static int option_interactive;
>
> Depending on how many options you add, you may want to use one global
> static int called 'flags' or similar, and use bit flags instead of a
> bunch of ints. From the program 'fdupes' in one of my GitHub repos,
> snipped up for brevity:
>
> #define ISFLAG(a,b) ((a & b) == b)
> #define SETFLAG(a,b) (a |= b)
> #define F_RECURSE 0x0001
> #define F_HIDEPROGRESS 0x0002
> etc. etc.
> ...
> switch (opt) {
> case 'q':
> SETFLAG(flags, F_HIDEPROGRESS);
> break;
> ...
> if (ISFLAG(flags, F_RECURSEAFTER)) ...
> if (!ISFLAG(flags, F_HIDEPROGRESS)) ...
>
> I don't necessarily recommend using this code, it's just an example
> and it's written to be usable with more than one variable that holds a
> set of flags. I'd rewrite it like this for ELKS programs to make it
> cleaner:
>
> static int flags;
> #define ISFLAG(a) ((flags & a) == a)
> #define SETFLAG(a) (flags |= a)
> #define F_QUIET 0x0001
> etc.
> ...
> if (strcmp(argv[i], "-q")) SETFLAG(F_QUIET);
> if (!ISFLAG(F_QUIET)) fprintf(stderr, "copied %d files\n", count);
>
> ---
> + if(option_verbose) {
> + printf("copied '%s' -> '%s'\n", srcname, destname);
> + }
>
> Firstly, the output from GNU 'cp' looks like this:
>
> $ cp -v foo/* bar/
> 'foo/1' -> 'bar/1'
> 'foo/2' -> 'bar/2'
> 'foo/3' -> 'bar/3'
>
> I'd also prefer one-liners such as this with no multi-line 'else'
> clause to be collapsed together (it makes the code easier to read and
> avoids 'brace hell'). I also prefer that if, for, and while statements
> have a space between the word and the (condition) part for the sake of
> code readability. Write it like this instead:
>
> if (option_verbose) printf("'%s' -> '%s'\n", srcname, destname);
>
> Next:
>
> + // set defaults for options //
> + option_no_overwrite = 0;
> + option_verbose = 0;
> + option_interactive = 0;
>
> The comment is unnecessary since it's obvious that "options are being
> initialized to zero" by the code itself. This is self-commenting code.
> I'd also prefer to see the variable names shortened a bit ('o_' or
> 'opt_' instead of 'option_' for example) and as mentioned previously,
> it's probably better to use the bit flags method mentioned previously
> if you're only using the variables as a yes/no flag.
>
> + arg_counter = 1; // start at argv[1]
> + options_counter = 0;
> + while(arg_counter < argc) {
> + if(!strcmp(argv[arg_counter], "-n")) {
> + options_counter++;
> + option_no_overwrite = 1;
> + }
> + if(!strcmp(argv[arg_counter], "-v")) {
> + options_counter++;
> + option_verbose = 1;
> + }
> + if(!strcmp(argv[arg_counter], "-i")) {
> + options_counter++;
> + option_interactive = 1;
> + }
> + if(!strcmp(argv[arg_counter], "-h")) goto usage;
> + arg_counter++;
> + }
>
> - if ((argc > 3) && !dirflag) {
> - fprintf(stderr, "%s: not a directory\n", lastarg);
> + if((argc > (3+options_counter)) && !dirflag) { // can't copy more
> than one src-files into one dst-file
>
> Ouch. Rather than try to explain the problems in detail, let me
> rewrite it.
>
> arg_cnt = 1; // start at argv[1]
> opt_cnt = 0;
> while(arg_cnt < argc) {
> if (!strcmp(argv[arg_cnt], "-n")) {
> opt_noclobber = 1;
> } else if (!strcmp(argv[arg_cnt], "-v")) {
> opt_verbose = 1;
> } else if (!strcmp(argv[arg_cnt], "-i")) {
> opt_interactive = 1;
> } else if (!strcmp(argv[arg_cnt], "-h")) goto usage;
> else break;
> opt_cnt++;
> arg_cnt++;
> }
>
> /* If multiple files specified, last one must be a directory */
> if ((argc > (opt_cnt + 3)) && !dirflag) {
>
> This rewrite combines all the option count increments into one
> instance which reduces code size and stops parsing options at the
> first non-option string. The comparison on the last line is more
> readable and it is clearer that it is "comparing total argument count
> to the number of options specified plus 3." The 'if-elseif-else' tree
> stops processing once a valid option is hit and "falls through" once a
> non-option is hit. It could (and probably should) be improved by
> handling the combined option case such as 'cp -iv src dest' but this
> is sufficient for now as long as the usage text implies that option
> flags must be specified separately.
>
> + copy_count = 0;
> + file_count = 0;
> + while (argc-- > (2+options_counter)) {
> + srcname = argv[options_counter+1+file_count];
>
> I've spent a lot of time cleaning these compressed mathematical
> expressions out of the elkscmd code; let's not introduce more of them
> (also shorten variable names while retaining their meaning, this is a
> good example for why to use shorter names):
>
> + copy_cnt = 0;
> + file_cnt = 0;
> + while (argc-- > (opt_cnt + 2)) {
> + srcname = argv[opt_cnt + 1 + file_cnt];
>
> Next,
>
> - if (dirflag) destname = buildname(destname, srcname);
> + if(dirflag) destname = buildname(destname, srcname);
>
> Don't do this, if no other reason than the fact that 'if' is not a
> function call. I understand that it comes down to individual coding
> style and the compiler doesn't care if you write
>
> if((something+(q/2))==argv[x+2+y*z[aaa]])usage();
>
> but it is hard to read and readability is very important. I believe
> that 'if', 'while', and 'for' shouldn't look like function calls in
> code since they are actually C control statements. Using a space makes
> those control statements immediately obvious, whereas if() and while()
> and for() look like function calls when skimming the code.
>
> - if (!copyfile(*++argv, destname, 0)) goto error_copy;
> + retval = copyfile(srcname, destname, 0);
> + if((!retval) && (!(retval == NOT_OVERWRITING))) goto error_copy;
>
> Remember the rule about silent success mentioned earlier? This change
> can be eliminated because of it. Even if it wasn't, though, this code
> could be simplified and made more readable in one shot:
>
> + retval = copyfile(srcname, destname, 0);
> + if (retval != 1) goto error_copy;
>
> I'd actually recommend reversing the return values in copyfile(),
> using nonzero as an error return instead (most C library calls seem to
> use this convention and consistency with that is rarely a bad thing)
> so you could just go:
>
> + if (retval) goto error_copy;
>
> Next,
>
> + if(option_verbose) printf("copy-count = %d\n", copy_count);
>
> This shouldn't be done. Here's GNU coreutils 'cp' in action:
>
> file_utils $ cp -v foo/* bar/
> 'foo/COPYING' -> 'bar/COPYING'
> 'foo/COPYING.sash' -> 'bar/COPYING.sash'
> 'foo/Makefile' -> 'bar/Makefile'
> 'foo/README' -> 'bar/README'
> 'foo/WARRANTY' -> 'bar/WARRANTY'
> ...snip...
>
> If someone is interested in knowing a "copy-count" for some reason,
> they'll almost certainly do this instead:
>
> file_utils $ cp -v foo/* bar/ | wc -l
> 46
>
> Your helpful message will throw this off and make it not work as
> expected. A shell script author will have to introduce an incompatible
> kludge to their script to make up for 'cp' misbehaving, i.e.
>
> CNT=$(cp -v $A/* $B/ | wc -l)
> CNT=$((CNT - 1)) # This shouldn't be necessary...
>
> Another problem is that you've declared a static global 'copy_count'
> variable and you're introducing a local 'copy_count' variable as well.
> It needs to be called something else for the sake of avoiding
> programmer confusion. However, assuming you eliminate the "copy-count"
> message I just talked about along with its local variable counterpart,
> the global 'static unsigned int copy_count' is set (copy_count++) but
> not used throughout the entire program and can be discarded.
>
> If you ever want an "official" reference for how a particular elkscmd
> program "should" behave, take a look at the POSIX standards:
>
> http://pubs.opengroup.org/onlinepubs/9699919799/
>
> Whew, that took me a long time to write. I hope it's helpful to you
> and everyone else on the list. Thanks for your impressive work so far!
> I'm looking forward to your finalized [PATCH] submissions. :-)
>
> -Jody
> --
> To unsubscribe from this list: send the line "unsubscribe linux-8086" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html