Hallo Jan und David & Mitleser, On Fri, Oct 08, 2004 at 12:54:05AM +0200, Jan Trippler wrote:
Am Donnerstag, 7. Oktober 2004 20:00 schrieb Daniel Lord: [...]
kleines C Programm (siehe unten)
Du hast es so gewollt ;-) Ich hoffe Du nimmst es mir nicht übel, wenn ich mal ein wenig in Deinem Code rumwildere.
ich lerne gerne was dazu.
<mailsig-02.c> [...] Setze defines immer in Klammern, wenn sie nicht nur aus einem einzigen konstanten Wert bestehen. Du weisst nie, wo sie später verwendet werden ;-) char ups[SIGWIDTH*10]; /* expandiert zu ups[80+5*10]=ups[130] */
der Rat ist gut und wird in Zukunft befolgt ;)
#define SIGLEN 6*(80+5) /* ugly and has to be less than
warum nicht so (gleich mal mit mir genehmen Werten ;-): #define SIGWIDTH (72+1) #define SIGLEN (4*SIGWIDTH)
weil das nur mit Klammer geht *g*
unsigned long signatur=1; /* overkill at the moment */ unsigned long randnum=-1;
Ist beides IMHO Overkill. Die Funktion rand(), die Du unten benutzt, liefert nur int. Damit kann auch ein rand()%signatur nie mehr als einen int liefern.
Der Kommentar war auch für beides und noch ein bischen mehr gedacht ;)
struct sig_data { unsigned char *data; }; struct sig_data sig[MAXSIGS];
Warum eine Struktur, wenn Du nur ein Array aus *char benötigst?
Das diente nur der besseren Erweiterbarkeit. Eine Struktur benötigt an sich keinen Speicherplatz. Und da ich das Programm irgendwann evtl. noch um Zusatzfunktionen aufstocken wollte... Deshalb struct.
/* Handle the argument */ #ifdef JUSTMYEXE if (argc != 1){ fprintf(stderr," use the source luke\n"); return EXIT_FAILURE; } #else if (argc != 2){ fprintf(stderr," usage: %s randomsig.file\n",argv[0]); return EXIT_FAILURE; } #endif
Diese ifdefs würde ich anders lösen: mailsig_02 [randomsig.file] also wahlweise mit oder ohne Argument aufzurufen (s. u.)
jein. Es ging mir darum ein möglichst kleines und schnelle Programm zu haben, das ausserdem einen möglichst keinen Speicherfootprint hat. Deshalb hab ich in das binary nur das reingepackt was ich wirklich brauchte. (Auch wenn auf heutigen Rechnern ein if mehr oder weniger kein Beinbruch ist.)
#else if ((fd=fopen(argv[1],"ro")) == NULL){ #endif
"ro"? - lt. meiner Manualpage gibt es nur "r" für den read-only Modus.
Ouch. Da war "rb" der Vater des Gedanken. Wobei das b unter Linux auch wieder bedeutungslos ist. Interessanterweise funktioniert das bei meinem GCC 3.3.4 ohne Probleme *grübel*
Ab hier habe ich ein grundsätzliches Problem mit Deinem Code.
*Seufz*
Warum liest Du die gesamte Datei in den Speicher (und schlägst Dich mit malloc() und den damit verbundenen Risiken rum), nur um die maximale Anzahl von Signaturen zu ermitteln? Ich halte das für unnötig. Meine Version habe ich unten angehängt.
Es ging mir weniger um die maximale Anzahl von Signaturen als vielmehr darum langsame Plattenzugriffe zu minimieren. D.h. ich lese die Datei nur ein einziges mal von der Platte (Diskcache etc. hab ich nicht berücksichtigt) Wäre aber glaube ich interessant mal einen Vergleich zu haben was schneller ist malloc oder zweimal lesen. (mit diskcache fürchte ich fast; deine Lösung :)
if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
while (fgets(tmp, SIGWIDTH, fd) != NULL) { if (emptyline(tmp) != 0) { strcat(sig[signatur].data,tmp);
Was passiert, wenn eine Zeile länger als SIGWIDTH-1 ist? Dann fehlt Dir das abschliessende \n am Zeilenende und die aktuelle Zeile wird in der Ausgabe an die davor gehende angehängt. Damit ist Deine Prüfung auf max. Zeilenlänge obsolet.
Das ist leider richtig.
Schlimmer: Du prüfst nicht, wie lang der Inhalt von .data bereits ist. Eine Signatur mit mehr als 6 Zeilen a 84 Zeichen würde wahrscheinlich einen Segmentation Fault bewirken, da in nicht initialisierten Speicher geschrieben wird.
Nein, malloc wird vorher immer ausgeführt. Die Signatur fällt später allerdings durchs Raster. [*]
} else { /* Allocate memory for the next signature */ signatur++; if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
Ähm - was machst Du, wenn mehrere leere Zeilen nacheinander kommen? Dann hast Du leere Signaturen.
hmm, doppelte Leerzeilen waren nicht vorgesehen :( Sollte ich wohl noch was einbauen :)
} }
/* if first malloc failes the app probably segfaults later. but may also end without problems so no exit here. */ failed: fclose(fd);
Hm. Ein "Könnte schiefgehen, muss aber nicht* halte ich für nicht so glücklich. Zumindest solltest Du nach einem abgebrochenen malloc() signatur um 1 verringern, damit Du nicht in uninitialiserte Bereiche kommst.
Die Lösung mit signature-- ist gut.
/* get a random number between 1 and signatur */ randnum = rand()%(signatur)+1;
Ja David, da krieg ich bei doppelten Abfragen pro Sekunde die gleiche Signatur. Das sehe ich, da die Signaturdatei bei mir userspezifisch ist, aber nicht als Problem.
/* make sure you don't get flamed :) */ if (strlen(sig[randnum].data) > SIGLEN) {
Wie soll das gehen? Du initialisierst doch .data nur mit SIGLEN+1 und wenn die Signatur länger ist, dann gibts oben einen Buffer Overflow. Also besser gleich oben abfangen.
[*] <- hier ist später Nein, es gibt keinen Overflow. Es wird weiterhin Speicher zugewiesen. Und hier wird nur getestet, ob die Signatur so halbwegs als Signatur durchgeht. Das Problem mit dem \n hast du ja oben schon gefunden.
/* Free the allocated memory */ for (count=1; count <= signatur; count++) { free(sig[count].data); }
Noch ne Kleinigkeit: Warum nutzt Du nicht in Deiner Struktur den Index ab 0? Du würdest Dir eine Menge +1 usw. ersparen :-)
Öhm PEBCAK...
So, nun meine Version. Ich lese die Datei 1,x mal zugunsten einer einfacheren Programmstruktur (und nehme dafür ein paar Millisekunden längere Programmlaufzeit in Kauf). Man kann auch da sicher noch ein paar Dinge optimieren, aber als erster Wurf ;-) Bevorzugen würde ich aber eigentlich immer ein Modell, bei dem das Dateiformat der Signaturdatei bereits beim Erzeugen validiert wird (siehe die anderen Mails im Thread):
Ja, nur hat mich das mit der Validierung gestört. Nach jedem Einfügen einer Signatur eine neue DB erstellen... Na gut, lässt sich auch mit ein bischen bash erledigen und ich denke ich werde wohl in die Richtung gehen. [...]
sigfile = (argc == 1) ? strdup (MYSIGFILE) : strdup (argv[1]); [...]
nett, wenn ich sowas auch mal aus dem FF schreibe, kann ich wohl sagen ich kann ein bischen C ;) Bis das soweit ist poste ich hier halt ab und an ein paar C Schnipsel und lass sie euch zerfleddern. Dann kann ich das auch irgendwann. Greetings Daniel -- Stadtplaner sind Menschen, die Radwege schaffen, indem sie Gehwege halbieren.