Moin, 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.
[...] #include #include #include #include #include #define MAXSIGS 10000 #define SIGWIDTH 80+5 /* more is abuse */
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] */
#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)
#ifdef JUSTMYEXE #define MYSIGFILE "/home/user/.mutt/randomsigs" #endif [...] short emptyline (char *string) { unsigned short l;
if (string[0] == '\n') return 0;
Unnötig, wird mit der for-Schleife auch erledigt.
for (l=0; l < (strlen(string)-1); l++) {
Nur eine Kleinigkeit: Du gehst mit l bis zu strlen(string)-1. Warum? Die Berechnung ist unnötig, das letzte Zeichen ist bei fgets eh ein \n (es sei denn, die Zeile ist zu lang - siehe unten) - ändert also nichts am Ergebnis. strlen(string) reicht.
if ((string[l] != ' ') && (string[l] != '\t')) { return 1; } } return 0; }
Ich würde es wie unten beschrieben machen.
int main (int argc, char *argv[]) { FILE *fd;
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.
unsigned long count=0; unsigned char tmp[SIGWIDTH];
struct sig_data { unsigned char *data; }; struct sig_data sig[MAXSIGS];
Warum eine Struktur, wenn Du nur ein Array aus *char benötigst?
/* 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.)
/* Initialise random number generator */ srand (time (0)); /* init random */
/* Open the signaturefile for reading */ #ifdef JUSTMYEXE if ((fd=fopen(MYSIGFILE,"ro")) == NULL){ #else if ((fd=fopen(argv[1],"ro")) == NULL){ #endif
"ro"? - lt. meiner Manualpage gibt es nur "r" für den read-only Modus.
perror("fopen"); return EXIT_FAILURE; }
Ab hier habe ich ein grundsätzliches Problem mit Deinem Code. 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.
/* Read the signature file and sort it into the sig_data struct */ 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. 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.
} 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.
} }
/* 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.
/* get a random number between 1 and signatur */ randnum = rand()%(signatur)+1;
/* 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.
strcpy(sig[randnum].data, "Probably PEBCAK. Signature way to long\n"); }
/* Write random signatur to stdout */ printf("%s",sig[randnum].data);
/* 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 :-)
return 0; }
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):
<schnipp>
#include