Code Review für ein Hexdump-Programm

Wutz

stream wird nicht geschlossen
perror(argv[1]) wäre besser
nur C99
%02x wäre besser
if( !(counter%5) ) wäre üblicher

Bashar

Nirvana134 schrieb:

while(!feof(stream)) {
            hex=fgetc(stream);
            printf("%3x ",hex);
            counter++;
            if(counter == 5) {
                printf("\n");
                counter = 1;
            }
        }

Du fragst EOF falsch ab. Das EOF-Flag wird erst dann true (und damit gibt auch feof true zurück), wenn bereits versucht wurde, über das Ende der Datei hinweg zu lesen. Das heißt, erst NACH fgetc weiß man, ob dieser Leseversuch erfolgreich war oder nicht. Du versuchst feof vorher schon aufzurufen, in der Hoffnung, es sagt dir, ob du fgetc aufrufen darfst.
Lösung: Den Rückgabewert von fgetc mit der Konstanten EOF (aus stdio.h) vergleichen und entsprechend reagieren, feof() brauchst du nicht.
Im übrigen erklärt das auch die Ausgabe von ffff, EOF hat nämlich in der Regel den Typ int und den Wert -1.

Nirvana134

ok, ich hab die Sachen korrigiert(stream nicht geschlossen war ein
Flüchtigkeitsfehler).

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

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

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

int main(int argc, char **argv) {

    /*Bin gets printed to stdout and counter counts when to print a newline*/
    unsigned int hex;
    int counter=1;
    /*Test if enough arguments are given*/
    if(argc < 2) {
        printf("missing arguments\n");
        printf("How to use: %s <filename>\n", argv[0]);
        exit(1);
    }

    FILE *stream;
    stream = fopen(argv[1],"rb");

    /*Test if the file exists*/
    if(stream==NULL) {
        perror(argv[1]);
        exit(2);
    }
    /*Reads the file and prints as hex*/
    else {
        while((hex=fgetc(stream))!=EOF) {
            printf("%02x ",hex);
           counter++;
            if(counter == 5) {
                printf("\n");
                counter = 1;
            }
        }
    }
    printf("\n");/*Newline after the end*/
    fclose(stream);

    return 0;
}

so besser? Ich werd es noch weiter verändern, wie Zeilenangabe und wahrscheinlich mehr Bytes pro Zeile anzeigen lassen malschauen. Vielleicht auch mit ncurses als Hex-Editor falls ichs hinkrieg.

Nirvana134 schrieb:

Vielleicht auch mit ncurses als Hex-Editor falls ichs hinkrieg.

Dann sieh mal zu das du erstmal die gesamte Datei in den Speicher bekommst.

Nirvana134

Ja, bin grad dabei, wird nur für mich etwas schwierig wenn ichs mit Parametern öffnen möchte(bin ja offensichtlich Anfänger). Muss aufpassen das der Code nicht total unlesbar wird. Ich poste mal die nächste "Version", wenn ich fertig bin.

Nirvana134

So hier ist meine neue Version, sieht nicht gut aus. Der Aufbau muss komplett umstrukturiert werden und es ist Ressourcenverschwendend, allerdings kann man immerhin es jetzt in eine Textdatei speichern. Die for schleife ist momentan überflüssig, da ichs völlig falsch programmiert habe damit sie mir was nützt. Aber mal schauen wie es demnächst aussieht. Hier der Code von der zweiten Version:

*************************************
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 function gets called if hexdump is called with -s parameter, instead to write so stdout, it saves it in hexdump*/
void writehextofile(FILE *stream, FILE *hexstream, int counter) {
    unsigned int hex;
    while((hex=fgetc(stream))!=EOF) {
        fprintf(hexstream,"%02x ",hex);
        counter++;
        if(counter == 15) {
            fprintf(hexstream,"\n");
            counter = 1;
        }
    }
    fclose(stream);
}

/*Selfexplaining function*/
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;
    /*i is for hexdump opened with parameters and counter counts when a
    a new line should appear*/
    int i, counter=1;

    /*Test if enough arguments are given*/
    if(argc < 2) {
        printf("Arguments are missing\n");
        printf("How to use: %s <filename>\n", argv[0]);
        printf("Type \"%s --help\" for help\n", argv[0]);
        exit(1);
    }

    FILE *stream, *hexstream;
    /*File which gets read*/
    if(argv[1][0] != '-') {
        stream = fopen(argv[1],"rb");
    }
    /*Test if the file exists*/
    if(stream==NULL && argv[1][0] != '-') {
        perror(argv[1]);
        exit(2);
    }
    /*Reads the file and prints as hex, no parameters passed to hexdump*/
    else if(argc==2 && argv[1][0] != '-') {
        printf("%08d\t",line);
        while((hex=fgetc(stream))!=EOF) {
            printf("%02x ",hex);
            counter++;
            if(counter == 15) {
                line++;
                printf("\n%08d\t",line);
                counter = 1;
            }
        }
        fclose(stream);
        printf("\n");/*Newline after the end*/
    }
    /*Parameters for hexdump*/
    else if(argc>=2) {
        for(i=1;i < argc && argv[i][0] == '-' && argv[i][1] != '\0';i++) {
            if(!strcmp(argv[i], "-s")) {
                if((hexstream=fopen(argv[i+1],"r"))!=NULL) {
                    printf("File %s does already exist\n", argv[i+1]);
                    fclose(hexstream);
                    exit(3);
                }
                /*File gets created if it does not already exist*/
                else {
                    hexstream=fopen(argv[i+1],"w");
                    stream=fopen(argv[3],"r");
                    writehextofile(stream,hexstream,counter);
                }
            }
            /*Print help message on how to use hexdump*/
            else if(!strcmp(argv[i], "-h"))
                helptext(argv[0]);
            else if(!strcmp(argv[i],"--help"))
                helptext(argv[0]);
        }

    }
    return 0;
}

Ich weiß der Code ist schlecht, aber ich hatte einen kleinen Fehler zu beheben wofür ich zuviel Zeit verschwendet habe, deswegen atte ich keine Zeit zum umstrukturieren

cooky451

Zeile 22 - Du solltest dir den Modulo Operator (%) mal angucken
Und was machst du eigentlich wenn der Dateiname mit einem - anfängt?

Edit:
Du solltest die Dinge nicht unnötig kompliziert machen. Du brauchst keine großartigen Startparameter, wenn nichts übergeben wird, zeigst du einfach die Hilfe an. --help und das Dateinamenproblem fallen dann schon mal weg. Eine output Datei musst du auch nicht selbst öffnen, man kann mit dem OS stdout ja eh einfach umleiten (und auf einer Waschmaschine braucht man kein Hexdump).

Mit ein paar Zeilen Code bekommt man das dann schon ganz nett hin:

int main(int argc, char *argv[])
{
  FILE *infile;
  int c, counter = 0;

  if (argc < 2)
  {
    const char *p = strrchr(argv[0], '\\');
    if (!p++)
      p = argv[0];
    printf("usage: %s file_name\n", p);
    return 0;
  }

  infile = fopen(argv[1], "rb");
  if (!infile)
    perror(argv[1]), exit(-1);

  while ((c = fgetc(infile)) != EOF)
  {
    if (counter++ % 16 == 0)
      printf("\n%08X ", counter - 1);
    printf("%02X ", c);
  }

  fclose(infile);
  return 0;
}

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;
}