HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Showing or altering the timestamp header on gzip archives

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
headertheshowingarchivestimestampgzipaltering

Problem

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
#include
#include
#include
#include
#include
#include

#include

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'

Solution

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.

Code Snippets

argc -= optind;
argv += optind;

for (int i=0; i < argc; ++i) {
    gziptimetravel(argv[i], &options);
}
if ((fp = fopen(filesrc, (settimeflag) ? "r+b" : "r")) == NULL) {
    perror(filesrc);
    exit(EXIT_FAILURE);
}
fprintf(stderr, "%s%lu%s\n", "Only read ", gheaderbytes, " bytes");
fprintf(stderr, "Only read %lu bytes\n", gheaderbytes);

Context

StackExchange Code Review Q#28217, answer score: 4

Revisions (0)

No revisions yet.