5
\$\begingroup\$

This program will show or alter the timestamp header on gzip archives. It was created for people who have issues using hash functions to compare gzip archives as the timestamps would be different. I want my code to really speak its intention without having to use comments outside of initialization (unless it is better). What can I do to improve this little guy?

gziptimetravel GitHub repository

Here is the bulk of the program. The only other file is a cmake generated file for versioning (the header).

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <ctype.h>

#include <gziptimetravel.h>

void displayHelp(void);
void displayVersion(void);
time_t getTime(const unsigned char vals[4]);
void printTime(const time_t t);


int main(int argc, char ** argv) 
{
    int printtimeflag  = 0; /*do we pretty print?*/
    int settimeflag    = 0; /*do we alter time on file*/
    int grabfrominflag = 0; /*are we getting input from stdin?*/
    int fileflag       = 0; /*is a file set*/

    const char * filesrc = NULL; /*path to input file passed*/
    char newtime[11]; /*passed in time (since epoch)*/
    char * tnewtime; /*temporary newtime for checking if newtime->ntime conversion succeeded*/
    size_t newtimelength; /*length of newtime buffer*/ 
    unsigned long ntime; /*the value of newtime*/
    FILE *fp; /*input file*/
    unsigned char gheaderbuffer[8]; /*buffer to hold header of gzip input file*/
    size_t gheaderbytes; /*how many bytes read into gheaderbuffer*/

    const unsigned char ID1 = 0x1f; /*IDentifier 1 (for gzip detection of file)*/
    const unsigned char ID2 = 0x8b; /*IDentifier 2*/

    int i; /*for loop iterator*/
    int c; /*option iteration*/
    while((c = getopt (argc, argv, "ps:S-:")) != -1) {
        switch(c) {
            case 'p':
                printtimeflag = 1;
                break;
            case 's':
                settimeflag = 1;
                strncpy(newtime, optarg, sizeof(newtime)-1);
                newtime[sizeof(newtime)-1] = '\0';
                break;
            case 'S':
                settimeflag = 1;
                grabfrominflag = 1;
                break;
            case '-':
                if(strcmp(optarg, "help") == 0) {
                    displayHelp();
                    exit(0);
                }
                if(strcmp(optarg, "version") == 0) {
                    displayVersion();
                    exit(0);
                }
                break;
            case '?':
                if(optopt == 's')
                    fprintf (stderr, "Option -%c requires an argument.\n", optopt);
                else if(isprint (optopt))
                    fprintf (stderr, "Unknown option `-%c'.\n", optopt);
                else
                    fprintf (stderr, "Unknown option character `\\x%x'.\n", optopt);

                fprintf(stderr, "Try gziptimetravel --help\n");
                exit(1);
            default:
                exit(1);
        }
    }

    for (i = optind; i < argc; i++) {
        if(!fileflag) {
            fileflag = 1;
            filesrc = argv[i];
        }
    }

    if(!fileflag) {
        fprintf(stderr, "Missing file operand (\"gziptimetravel --help\" for help)\n");
        exit(1);
    }

    fp = fopen(filesrc, (settimeflag) ? "r+b" : "r");
    if(fp == NULL) {
        fprintf(stderr, "%s%s\n", filesrc, ": No such file");
        exit(1);
    }

    gheaderbytes = fread(gheaderbuffer, sizeof(unsigned char), sizeof(gheaderbuffer), fp);
    if(gheaderbytes < sizeof(gheaderbuffer)) {
        fprintf(stderr, "%s%lu%s\n", "Only read ", gheaderbytes, " bytes");
        exit(1);
    }

    /* gzip's 8 byte header format:
     * bytes name desc
     * 1: ID1   -- (IDentification 1)
     * 1: ID2   -- (IDentification 2)
     * 1: CM    -- (Compression Method) 
     * 1: FLG   -- (FLaGs)
     * 4: MTIME -- (Modification TIME)
     */

    if(gheaderbuffer[0] != ID1 || gheaderbuffer[1] != ID2) {
        fprintf(stderr, "%s%s%s\n", "File:", filesrc, " is not a gzip archive\n");
        exit(1);
    }

    if(!printtimeflag && !settimeflag) {
        time_t mtime = getTime(gheaderbuffer+4);
        printf("%lu\n", mtime);
    }

    if(printtimeflag) {
        time_t mtime = getTime(gheaderbuffer+4);
        printTime(mtime);
    }

    if(settimeflag) {
        if(grabfrominflag) {
            fflush(stdout);

            if(!fgets(newtime, sizeof(newtime), stdin)) {
                fprintf(stderr, "Error reading from stdin");
                exit(1);
            }

            newtimelength = strlen(newtime);
            if(newtime[newtimelength-1] == '\n') newtime[--newtimelength] = '\0';
        }

        tnewtime = newtime;
        errno = 0;
        ntime = strtoul(newtime, &tnewtime, 0);
        if(errno != 0) {
            fprintf(stderr, "Conversion of time failed, EINVAL, ERANGE\n");
            exit(1);
        }
        if(*tnewtime != 0) {
            fprintf(stderr, "Conversion of time failed, pass an unsigned integer\n");
            exit(1);
        }

        fseek(fp, 4, SEEK_SET);
        fputc((ntime       & 0xFF), fp);
        fputc((ntime >> 8  & 0xFF), fp);
        fputc((ntime >> 16 & 0xFF), fp);
        fputc((ntime >> 24 & 0xFF), fp);
    }

    fclose(fp);
    return 0;
}

time_t getTime(const unsigned char vals[4])
{
    time_t mtime = (vals[0])       +
                   (vals[1] << 8)  +
                   (vals[2] << 16) +
                   (vals[3] << 24);
    return mtime; 
}

void printTime(const time_t t)
{
    char gheaderbuffer[100];
    strftime(gheaderbuffer, 100, "%Y-%m-%d %H:%M:%S", localtime(&t));
    printf("%s\n", gheaderbuffer);
}

void displayHelp(void)
{
    printf("Usage: gziptimetravel [OPTIONS...] SOURCE\n"
           "Set or view timestamp of gzip archives\n"
           "\n"
           "  -p                       print formatted timestamp to stdout\n"
           "  -s [seconds from epoch]  set timestamp of file\n"
           "  -S                       set timestamp of file from stdin\n"
           "\n"
           "If no flags other than file are given gziptimetravel will output inputs timestamp as seconds since epoch\n"
           "\n");
    printf("EXAMPLES:\n"
           "  gziptimetravel input.tar.gz\n"
           "                      Print input.tar.gz's timestamp\n"
           "\n"
           "  gziptimetravel -s0 input.tar.gz\n"
           "                      Set input.tar.gz's timestamp to January 1st 1970\n"
           "\n"
           "  echo \"123456789\" | gziptimetravel -S input.tar.gz\n"
           "                      Set input.tar.gz's timestamp to 1973-11-29 November 29th 1973\n"
           "\n"
           "\n");
    printf("Report gziptimetravel bugs to whackashoe@gmail.com\n"
           "gziptimetravel homepage: <https://github.com/whackashoe/gziptimetravel/>\n");
}

void displayVersion(void)
{
    printf("%s%d%c%d\n", "gziptimetravel ", GZIPTIMETRAVEL_VERSION_MAJOR, '.', GZIPTIMETRAVEL_VERSION_MINOR);
}
\$\endgroup\$
4
\$\begingroup\$

An interesting program. A few comments

  • there is too much in main. You could usefully split it into several separate functions, one of which would handle argument processing. Admittedly, your use of getopt from main means that you can set local variables in main to control the program - if option handling is separated out, you must either pass the option variables in, define and pass an options structure holding those variables, or use globals (preferably not). Despite this inconvenience, I would still separate option handling.

  • The default of printing the time in seconds seems less useful than a default of printing the formatted time, with the value in seconds being available through an option.

  • I wasn't aware that one could use long options through getopt (as opposed to getopt_long) in that way. Clearly it works :-)

  • You print an extra message when a wrong option or missing argument is input but getopt already prints a message. You should supress this default message - see the getopt man-page - or not print your own.

  • You use your preferred name for the application in messages rather than using argv[0], which is perhaps more normal.

  • It would be better after processing options to adjust the argument variables and then process each remaining argument as an input file, rather than restricting input to just one file.

    argc -= optind;
    argv += optind;
    
    for (int i=0; i < argc; ++i) {
        gziptimetravel(argv[i], &options);
    }
    

    This means that everything from the fopen call downwards is extracted from main to a function, such as gziptimetravel and other functions.
    I would have function for each activity (is-header check, printing time seconds, printing formatted time, setting time, getting time from user) called from gziptimetravel

  • Using perror on failure of a system call (or indeed of strtoul) instead of rolling your own error messages would be more normal:

    if ((fp = fopen(filesrc, (settimeflag) ? "r+b" : "r")) == NULL) {
        perror(filesrc);
        exit(EXIT_FAILURE);
    }
    

    Also your own messages are unusual, for exammle,

    fprintf(stderr, "%s%lu%s\n", "Only read ", gheaderbytes, " bytes");
    

    is usually written

    fprintf(stderr, "Only read %lu bytes\n", gheaderbytes);
    
  • some of your variable names are too long for me (gheaderbuffer == header, gheaderbytes == nbytes). This is brought about by having everything in one big function and defining all variables at the start. Splitting into several functions means you can reduce variable name sizes without becoming uninteligible. Defining all variables at the top of main is too early for many. It is generally preferable to define variables only when/where you need them. For-loop variables can be defined within the loop.

  • Trivial point: sizeof(unsigned char) is 1 by definition.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot, this really helped. I've updated my repository with all changes and a bit more, minus one which I have a question on: why do you think that parsing the options should be in a separate function? I tried it, then realized I was just calling checkArgs and exiting, felt as though it was unnecessary indirection? I'd like to hear your thoughts on it. \$\endgroup\$ – jett Jul 8 '13 at 5:28
  • 1
    \$\begingroup\$ Your updated code (in github) looks better. I think it would be worth re-submitting it as a new question (they are free :-) to get extra comments as there are many style issues that I would comment upon. On separating option handling into a function, there is no right and wrong. Much programming is about personal taste and readability. To my taste, main is rather long - separating option handling would make it clearer. Others may disagree. \$\endgroup\$ – William Morris Jul 8 '13 at 12:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.