patterncMinor
Showing or altering the timestamp header on gzip archives
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'
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
separate functions, one of which would handle argument processing.
Admittedly, your use of
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
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
to
-
You print an extra message when a wrong option or missing argument is input
but
message - see the
-
You use your preferred name for the application in messages rather than
using
-
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.
This means that everything from the
from
I would have
function for each activity (is-header check, printing time seconds,
printing formatted time, setting time, getting time from user) called from
-
Using
of rolling your own error messages would be more normal:
Also your own messages are unusual, for exammle,
is usually written
-
some of your variable names are too long for me (
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
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:
-
there is too much in
main. You could usefully split it into severalseparate functions, one of which would handle argument processing.
Admittedly, your use of
getopt from main means that you can set localvariables 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). Despitethis 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 opposedto
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 defaultmessage - 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 extractedfrom
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) insteadof 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 havingeverything 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
mainis 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.