Code Review für ein Hexdump-Programm

Nirvana134

Ich weiß das man stdout umleiten könnte es ist aber denke ich schöner es in einem Programm zu haben(der Code ist zwar komplexer, aber naja).
Das mit der for-schleife hab ich gemacht, weil ich das Programm noch erweitern möchte. Das heißt mehr Parameter fürs speichern, mit oder ohne Zeilenangabe oder am besten die Zeilenangabe auf stdout und den Hexdump ins File etc.
Allerdings macht es auf diese weise keinen Sinn, da ich keine Flags benutze, sondern direkt in die Funktion springen.

Welchen Vorteil hat es wenn ich % anwende statt == ?

Nirvana134

Eine Frage hätt ich noch und zwar würd ich gern wissen wie ich aus dem Hexdump wieder die ursprüngliche Binärdatei herstelle. Das heißt ich müsste
die Leerzeichen und \n wieder löschen, aber wie mach ich aus den Hexcode wieder ne Binärdatei.

Entweder bei scanf() mit %x oder strtol() (der 3. Parameter ist wichtig))

cooky451

Nirvana134 schrieb:

Ich weiß das man stdout umleiten könnte es ist aber denke ich schöner es in einem Programm zu haben

Ich denke das nicht.

Nirvana134 schrieb:

Das mit der for-schleife hab ich gemacht, weil ich das Programm noch erweitern möchte.

Dann versuche möglichst die Parameterverarbeitung vom "eigentlichen" Programm zu trennen. Das sieht so alles sehr verwurschelt aus. Und da du den Dateinamen nicht wirklich gut eingrenzen kannst, solltest du einfach festlegen, dass dieser in argv[1] zu stehen hat. (Du weist ja nicht ob der Dateiname mit einem '-' anfängt.)

Nirvana134 schrieb:

Welchen Vorteil hat es wenn ich % anwende statt == ?

Die Frage solltest du noch mal überdenken. Wo habe ich das == ersetzt

Weist du überhaupt was der Modulo Operator macht? Falls nicht:
http://de.wikipedia.org/wiki/Division_mit_Rest

int main()
{
  for (int i = 0; i < 100; ++i)
    printf("%i\n", i % 7);
  return 0;
}

Ich denke so sollte klar werden, warum Modulo so toll ist

Nirvana134

@ cooky451 Wutz hat das == mit % ersetzt.
Ok, jetzt seh ich auch den Sinn, ich muss nicht counter immer einen neue Wert zuweisen. Alles klar, ich werds implementieren/ersetzen.

Ich werd das mit der for-Schleife neu programmieren so dass es diesmal besser läuft mit den Namen und Parametern etc.

Ist es in Ordnung wenn ich die, sagen wir nächsten 2 Versionen poste, oder bin ich dafür im falschen Forum?

@ cook451 Aber damit ich das richtig verstanden habe, mit dem OS stdout ist das so gemeint, dass ich im Terminal den stdout auf eine andere Datei weiterleite(pipeline) also mit | richtig?

Danke für eure Hilfe, freundliches Forum habt ihr.

cooky451

Nirvana134 schrieb:

@ cooky451 Wutz hat das == mit % ersetzt.

Nicht wirklich
Man kann statt
if (a != 0)
auch
if (a)
schreiben. Mann kann das Ergebnis einer Vergleichsoperation auch in einer Variablen speichern:
a = 0
a = a == 0
Jetzt wäre a wieder ungleich 0, da a == 0 ja true ist.

Nirvana134 schrieb:

Ist es in Ordnung wenn ich die, sagen wir nächsten 2 Versionen poste, oder bin ich dafür im falschen Forum?

Ich glaube das stört niemanden, wers nicht lesen will liests halt nicht

Nirvana134 schrieb:

@ cook451 Aber damit ich das richtig verstanden habe

Mit Windows wäre das z.B. myhexdump.exe > out.txt
Mir würde jetzt auch kein (relevantes) System einfallen, auf dem so etwas nicht geht. Wenn du dir eh die Mühe machst, einen langatmigen Startparameter-Parser zu schreiben, kannst du die Funktion natürlich noch einbauen, aber wirklich sinnvoll ist das nicht unbedingt..

Falls du das doch vorhast, so wäre das wahrscheinlich am praktischsten:

int main()
{
  FILE *outfile = stdout;
  if (parameter gesetzt)
    outfile = fopen(..);

  fprintf(outfile, ..);

  if (parameter gesetzt)
    fclose(outfile);
}

Nirvana134

Ok, danke so werd ichs mit den Parametern machen, sieht gut aus.

Nirvana134

Und noch ne Frage:
Hat eine Textdatei in Linux keine Metainformationen? Denn ich hab gelesen das bei fopen() unter Linux das b in "rb" ignoriert wird. Das heißt bei der Textdatei werden direkt 1Byte-Werte gespeichert welche als ASCII interpretiert werden?

cooky451

Ist unter Windows nicht anders. Textdateien haben nie Metainformationen. Notepad erkennt das Format auch nur durch statistische Analysen.

Aber das kann dir für ein Hexdump doch auch egal sein

Nirvana134

Naja, wenn ich die Hexdatei speichere ist sie ja eine Textdatei. Und die will ich wieder in die Binäre Form bringen. (Als Übung eigentlich gedacht)

cooky451

Nirvana134 schrieb:

Naja, wenn ich die Hexdatei speichere ist sie ja eine Textdatei. Und die will ich wieder in die Binäre Form bringen. (Als Übung eigentlich gedacht)

Du liest aber weiterhin Byte für Byte, da kann dir die Ordnung egal sein.

Das binäre Format bezieht sich auf die Zeilenendezeichen '\r' und/oder '\n'.

Die werden bei den verschiedenen OS unterschiedlich benutzt.
Bei Linux ist das '\n'
bei MacOS ist das '\r', auch bei OS/9
bei Windows sind das '\r\n' (beide).

Beim lesen mit "r" wird dann das/die Zeilenendezeichen in '\n' umgewandelt bzw.
beim schreiben mit "w" wird das '\n' in das/die passende Zeilenendezeichen umgewandelt.

Im binären Modus passiert das nicht. Du bekommst die Zeichen so wie sie in der Datei stehen.

Nirvana134

Ich hab hier die nächste Version, die Parameterabfrage mit for-Schleife hat mich fürs erste überfordert habs verworfen. Es schlanker zu machen lohnt sich eh nicht mehr von daher, war glaub ich ne gute Übung. Wenn mich jemand noch auf ein paar Fehler aufmerksam macht könnt ich immerhin noch was lernen. Hier der Code:

/*************************************
This program shows the hexcode
of any given program or file.
May be called a hexadecimaldump or hexdump.

*************************************/

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

/*More readable with TRUE and FALSE*/
#define TRUE 1
#define FALSE 0

/*This is the actual converting to hex and writing it to a file or stdout*/
void writehextostream(FILE *stream, FILE *hexstream, int counter,int saveflag) {
    unsigned int hex;
    while((hex=fgetc(stream))!=EOF) {
        fprintf(hexstream,"%02x ",hex);
        counter++;
        if(!(counter%5)) {
            fprintf(hexstream,"\n");
        }
    }
    fprintf(hexstream,"\n");
    fclose(stream);

    if(saveflag==TRUE)
        fclose(hexstream);
}

/*Selfexplaining function which helps on how to use hexdump*/
void helptext(char *programname) {
    printf("Usage: %s [OPTION] [FILE]\n", programname);
    printf("\t%s -s [NEWFILE]\tSaves the hexdump into [NEWFILE]\n",programname);
    printf("\t%s -h print this help message\n",programname);
}

int main(int argc, char **argv) {
    /*Hex gets printed to stdout and and line counts the lines*/
    unsigned int hex, line=1;
    /*counter counts when a
    a new line should appear*/
    int saveflag, counter=0;

    /*Test if enough arguments are given, otherwise terminate the program*/
    if(argc < 2) {
        printf("Arguments are missing\n");
        helptext(argv[0]);
        exit(1);
    }

    else if(argc>4) {
        printf("Too many arguments\n");
        helptext(argv[0]);
        exit(1);
    }

    FILE *stream, *hexstream;
    /*Parameters for hexdump*/
    if(!strcmp(argv[1], "-s")) {
        if((hexstream=fopen(argv[2],"r"))!=NULL) {
            printf("File %s does already exist\n", argv[2]);
           fclose(hexstream);
            exit(3);
        }
        /*File gets created if it does not already exist*/
        else {
            hexstream=fopen(argv[2],"w");
            stream=fopen(argv[3],"r");
            writehextostream(stream,hexstream,counter,saveflag=TRUE);
        }
    }
    /*Print help message on how to use hexdump*/
    else if(!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))
        helptext(argv[0]);
    else if(argc==2) {
        /*File which gets read*/
        stream = fopen(argv[1],"r");
        /*Test if the file exists*/
        if(stream==NULL) {
            perror(argv[1]);
            exit(2);
        }
        /*Without parameters (argc==2), hex gets written to stdout and lines won't be printed*/
        else {
            writehextostream(stream,stdout,counter,saveflag=FALSE);
        }
    }
    return 0;
}

Für die Parameter würde ich auf eine entsprechende Bibliothek zurückgreifen - es macht sich da bezahlt, standardisiertes Verhalten zu benutzen, um den Benutzer nicht unangenehm zu überraschen. POSIX definiert getopt, das ist ein guter Ansatzpunkt. Im WinAPI ist es leider nicht enthalten, aber es gibt frei verfügbare drop-in replacements beispielsweise hier und hier. Dokumentation hier.

Damit kann man die ganze Sache dann etwa so aufziehen:

#include <ctype.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <getopt.h>

#define ERROR_OPTION_GENERIC     (1)
#define ERROR_INVALID_LINE_WIDTH (2)
#define ERROR_CANT_OPEN_INFILE   (3)
#define ERROR_CANT_OPEN_OUTFILE  (4)
#define ERROR_OUT_OF_MEMORY      (5)

static char const *const error_messages[] = {
  "",
  "",
  "Ungültige Zeilenbreite\n",
  "Fehler beim Öffnen der Eingabedatei\n",
  "Fehler beim Öffnen der Ausgabedatei\n",
  "Speicherfehler\n"
};

void usage(char const *progname) {
  printf("Verwendung: %s [ -h ] [ -i DATEI ] [ -o DATEI ] [ -l ZAHL ]\n"
         "\n"
         " -h       : Diese Meldung ausgeben\n"
         " -i DATEI : Statt von stdin aus DATEI lesen\n"
         " -o DATEI : Statt nach stdout in DATEI schreiben\n"
         " -l ZAHL  : ZAHL Byte in einer Zeile darstellen (Default: 16)\n", progname);
}

int write_hexdump(FILE *in, FILE *out, int line_width) {
  unsigned char *line_buffer = malloc(line_width + 1);
  size_t i, j, r;

  if(!line_buffer) { 
    return ERROR_OUT_OF_MEMORY;
  }

  for(i = 0; (r = fread(line_buffer, 1, line_width, in)) > 0; i += line_width) {
    fprintf(out, "%08lx", (unsigned long) i);

    for(j = 0; j < r; ++j) {
      fprintf(out, " %02x", line_buffer[j]);
      line_buffer[j] = isprint(line_buffer[j]) ? line_buffer[j] : '.';
    }
    line_buffer[j] = 0;
    fprintf(out, "%*c%s\n", (line_width - (int) r) * 3 + 1, ' ', line_buffer);
  }

  free(line_buffer);
  return 0;
}

int main(int argc, char *argv[]) {
  int c;
  int status = 0;
  int line_width = 16;

  /* Wenn keine Dateioption angegeben wird, Standardein- bzw. -ausgabe benutzen */
  FILE *in  = stdin;
  FILE *out = stdout;
  char * in_name = NULL;
  char *out_name = NULL;

  while(status == 0 && (c = getopt(argc, argv, "hi:o:l:")) != -1) {
    switch(c) {
    case 'h':
      usage(argv[0]);
      return 0;
    case 'i':
      in_name = optarg;
      break;
    case 'o':
      out_name = optarg;
      break;
    case 'l':
      line_width = atoi(optarg);
      if(line_width < 1) {
        status = ERROR_INVALID_LINE_WIDTH;
      }
      break;
    case '?':
      return ERROR_OPTION_GENERIC;
    }
  }

  if(status == 0 && in_name) {
    in  = fopen(in_name , "rb");
    if(!in ) { status = ERROR_CANT_OPEN_INFILE ; }
  }

  /* Ausgabedatei zuletzt öffnen, damit im Fehlerfall nicht unnötigerweise
   * Dateien erstellt werden. */
  if(status == 0 && out_name) {
    out = fopen(out_name, "w");
    if(!out) { status = ERROR_CANT_OPEN_OUTFILE; }
  }

  if(status == 0)  {
    status = write_hexdump(in, out, line_width);
  }

  if( in && in  != stdin ) fclose(in );
  if(out && out != stdout) fclose(out);

  fputs(error_messages[status], stderr);
  return status;
}

Nirvana134

Danke, getopt.h ist genau das was ich brauche. Hab mich immer gefragt wie man das mit den Parametern am besten regelt. Gibt es einen Unterschied zwischen getopt()
in getopt.h und unistd.h, ich weiß nur das bei getopt.h (is von GNU richtig?) es noch die die funktion mit zwei dasches gibt (was ich nicht brauche momentan).

Irgendwelche Vorteile unistd.h oder getopt.h zu wählen?

getopt_long ist eh eine GNU-Erweiterung. Ansonsten scheint, wo ich grad noch mal in den POSIX-Standard kucke (der Link, den ich oben angegeben hatte), unistd.h eigentlich richtig zu sein, und meine Suns haben getopt.h auch gar nicht. Was die getopt.h-Funktion angeht, in der glibc bindet unistd.h getopt.h ein, und für den Linker ist das nachher sowieso ein und das selbe.

Also besser unistd.h, und für Windows ggf. halt ein #ifdef _WIN32-Sonderfall.