MoinMoin,
ich habe ein kleines Problem mit einem, in C geschriebenen Programm,
welches auf einer Linux-Kiste läuft. Dieses Programm stürzt ab und zu
ab, wenn ich (wahrscheinlich) diese Prozedur aufrufe:
// eventuell alte vorhandene Liste mit Playlisten loeschen
18
if (playlists != NULL) {
19
// an Anfang der Liste...
20
while (playlists->prev!=NULL) playlists=playlists->prev;
21
// ...und bis zum Ende loeschen
22
while (playlists != NULL) {
23
ptr = playlists->next;
24
free(playlists);
25
playlists = ptr;
26
}
27
}
28
// Liste mit Playlisten erzeugen...
29
for (data=mpd_database_playlist_list(mi); data!=NULL; data=mpd_data_get_next(data)){
30
if (playlists == NULL) {
31
playlists = malloc(sizeof(*playlists));
32
strcpy(playlists->name, data->playlist->path);
33
playlists->prev = NULL;
34
playlists->next = NULL;
35
} else {
36
ptr = playlists;
37
while (ptr->next != NULL) ptr = ptr->next;
38
prev = ptr;
39
ptr->next = malloc(sizeof(*ptr));
40
ptr=ptr->next;
41
strcpy(ptr->name, data->playlist->path);
42
ptr->prev = prev;
43
ptr->next = NULL;
44
}
45
}
46
}
Sinn dieser Prozedur ist es, Playlist-Informationen (in diesem Fall die
Namen) von MPD (Media Play Daemon) zu holen und diese in einer
dynamischen Liste abzulegen.
Zur initialen Anzeige wird diese Prozedur später verwendet (aber ich
vermute, bis hier her kommt das Programm im Absturzfall nicht...):
1
// ********************************************
2
void mpd_queue_output(void)
3
{
4
int y = 30;
5
6
if (playlists != NULL) {
7
put_string(10, y, "Playlists:", GREY, FONT_8X8);
8
y+=20;
9
put_string(10, y, playlists->name, LIGHT_GREY, FONT_8X16);
10
} else {
11
put_string_center(160, 80, "No playlists found!", LIGHT_GREY, FONT_8X16);
12
}
13
}
Es kommt nicht immer zum Absturz des Programms.
Sieht jemand in obigen Code-Fragmenten ein Problem, welches ich
übersehen habe?
Grüße & Danke Uwe
PS.: Mir ist bewußt, dass man für strcpy besser strncpy nehmen sollte,
werde ich auch nochmal ausprobieren... Aber wenn dies das Problem wäre,
müsste das Programm immer abstürzen und ich habe derzeit auch keine
Namen mit mehr als 30 Byte.
Hmm, ich würde ja mal überlegen, auf C++ umzusteigen, da musst du das
nicht selbst implementieren. Ansonsten:
Speicherleck:
// an Anfang der Liste...
while (playlists->prev!=NULL) playlists=playlists->prev;
was ist das hier? mpd_database_playlist_list
Sven schrieb:> Speicherleck:>> // an Anfang der Liste...> while (playlists->prev!=NULL) playlists=playlists->prev;>
warum?
Sven schrieb:> was ist das hier? mpd_database_playlist_list>
eine Funktion aus der libmpd
Grüße Uwe
gefährlich!
Nimm wennigstens strncpy um den Text zu kopieren. Wenn in path
tatsächlich ein Filesystem Pfad drinnen steht, dann sind 29 nutzbare
Zeichen nicht allzuviel.
Also, ich bin eine totale C-Pfeife, also seht es mir bitte nach, wenn
ich jetzt Blödsinn rede - aber müsste man die Zielbereiche für strcpy()
nicht irgendwie alloziieren, also mit einem neuen Array oder malloc oder
sowas? Und müsste man playlists.name nicht auch irgendwie beim Löschen
freigeben?
(OK, streicht obiges aus dem Protokoll, ich habe gerade die
struct-Definition nochmal gelesen, nachdem ich die Tomaten von meinen
Augen entfernt habe.)
Uwe Berger schrieb:> Sven schrieb:>> Speicherleck:>>>> // an Anfang der Liste...>> while (playlists->prev!=NULL) playlists=playlists->prev;>>> warum?
Speicherleck zwar nicht.
Aber es erhebt sich die Frage, wozu das überhaupt gut sein soll.
Dein Pointer playlists sollte sowieso IMMER auf den ersten Eintrag in
der Liste zeigen! Diesen Pointer gibt man nicht aus der Hand. Wenn du
einen Pointer auf so etwas wie eine aktuelle Playlist benötigst, den der
Benutzer bei einer Auswahl auf die nächste oder die vorhergehende setzen
kann, dann macht man sich dafür einen eigenen Pointer. Aber den
Root-Pointer auf den Anfang einer Liste gibt man nie leichtfertig aus
der Hand.
Der Rest ist ein wenig ungelenk programmiert, aber beim schnellen
Drüberschauen sieht es eigentlich nicht so schlecht aus.
Du kannst den Code massiv vereinfachen, indem du das Prinzip "Teile und
Herrsche" anwendest.
Zum einen ist das komplette Löschen einer Liste eine Operation, die sich
sicherlich eine eigene Funktion verdient hat.
Zum anderen kannst du die ganze Einfügeoperation dadurch vereinfachen,
dass du die beiden Teile "Erzeuge ein neues Listenelement" und "Hänge es
in die Liste ein" nicht ineinander verwebst, sondern getrennt
hintereinander ausführst. Des weiteren brauchst du auch diese ganze
komplizierte "Suche das Ende der Liste" Nummer nicht, denn du weisst ja,
welches das letzte Element in der Liste ist: Du hast es ja einen
SChleifendurchlauf vorher gerade eingefügt.
Im übrigen vermutest du mir ein bischen zu viel.
SO kommt man in der Programmierung nicht weiter.
Gerade wenn man mit dynamischen Datenstrukturen arbeitet, sind
Debug-Ausgaben oft das um und auf. Du hast eine Möglichkeit, dir
irgendwo Texte auszugeben? Dann benutze die auch, indem dir das Programm
mit ein paar printf (oder was du sonst so hast), ausgibt, was es gerade
macht, bzw. wie die Datenstruktur aussieht.
Karl Heinz schrieb:> Zum anderen kannst du die ganze Einfügeoperation dadurch vereinfachen,> dass du die beiden Teile "Erzeuge ein neues Listenelement" und "Hänge es> in die Liste ein" nicht ineinander verwebst, sondern getrennt> hintereinander ausführst.
Wobei auch der Teil: erzeuge ein neues Listenelement eine Funktionalität
ist, die man auch an anderen Stellen noch brauchen könnte und es sich
daher auch verdient hat, eine eigene Funktion zu bekommen.
pNew->name[sizeof(pNew->name)-1]='\0';// nur um sicher zu gehen
20
pNew->next=NULL;
21
pNew->prev=NULL;
22
}
23
24
returnpNew;
25
}
26
27
voidmpd_lsplaylists(MpdObj*mi)
28
{
29
MpdData*data;
30
structplaylist_struct*newItem;
31
structplaylist_struct*prevItem;
32
33
deletePlaylists();
34
35
for(data=mpd_database_playlist_list(mi);
36
data!=NULL;
37
data=mpd_data_get_next(data)){
38
39
// neuen Listeneintrag erzeugen
40
newItem=createPlaylistItem(data->playlist->path);
41
42
// und einhaengen
43
if(newItem){
44
if(playlists==NULL)
45
playlist=newItem;
46
else{
47
prevItem->next=newItem;
48
newItem->prev=prevItem;
49
}
50
51
// das jetzige Item wird im naechsten Schleifendurchlauf
52
// zum vorhergehenden Item des naechsten zu erzeugenden Items
53
prevItem=newItem;
54
}
55
}
56
}
Die einzelnen Funktionen deletePlaylists bzw. createPlaylistItem sind
jetzt banal testbar, bzw. so einfach aufgebaut, dass man sie leicht
überschauen kann und sich davon überzeugen kann, dass sie soweit erst
mal korrekt sind. Und auch die Funktion mpd_lsplaylists hat von der
extrahierten Funktionalität provitiert, indem sie kürzer und soweit
übersichtlicher geworden ist, dass man keine Kopfstände machen muss um
zu verstehen, wie sie funktioniert und sich davon überzeugen kann, dass
sie korrekt ist.
Code macht sich nicht von alleine übersichtlich. Da musst du schon
selber ran. Aber es lohnt sich. Vor allen Dingen bei dynamischen
Datenstrukturen. Das sinnvolle Aufteilen in Teilfunktionen ist dazu
einer der Schlüssel. Oft sind dann diese Teilfunktionen auch für sich
alleine nützlich, das muss allerdings nicht unbedingt so sein.
Und nicht vergessen:
Den Root-Pointer auf eine dynamische Datenstruktur gibst du niemals,
unter keinen Umständen, aus der Hand. Der zeigt immer auf das erste
(oberste) Element der Datenstruktur. Immer. Komme was da wolle. Wenn du
eine 'Markierung' innerhalb der Struktur brauchst, ein 'current Item',
dann machst du dir einen eigenen Pointer dafür. Das folgt schon alleine
daraus, dass du zwischen der Liste in ihrer Gesamtheit und den einzelnen
Items innerhalb der Liste eine logische Trennung ziehen willst, selbst
wenn beide den gleichen Pointer-Datentyp haben. Der Root-Pointer
repräsentiert die Liste in ihrer Gesamtheit, während ein Current Item
Pointer ein bestimmtes Element der Liste identifiziert. Das sind logisch
gesehen zwei komplett verschiedene Dinge.
Aber du wirst einen Teufel tun und für letzteres den Root-Pointer
benutzen.
MoinMoin,
Karl Heinz schrieb:> Im übrigen vermutest du mir ein bischen zu viel.> SO kommt man in der Programmierung nicht weiter.>> Gerade wenn man mit dynamischen Datenstrukturen arbeitet, sind> Debug-Ausgaben oft das um und auf. Du hast eine Möglichkeit, dir> irgendwo Texte auszugeben?>
...:-)..., da hast du natürlich recht und mache ich in der Regel auch.
Gestern hatte ich die Hardware nicht dabei gehabt und mir also
"theoretisch" den Kopf zerbrochen....
OK, jetzt keine Vermutungen, sondern Tatsachen --> das Problem tritt
nicht in meiner Listenverwaltung (bzw. dem nicht verwendeten strncpy)
auf, sondern genau in dieser Zeile:
1
for (data=mpd_database_playlist_list(mi); data!=NULL; data=mpd_data_get_next(data))
...und das beim ersten Durchlauf
Der MPD-Server scheint aus irgendeinem Grund nicht immer so zu
reagieren, wie ich es mir gedacht hatte. Also lag ich mit meiner
Vermutung ganz daneben und werde jetzt an einer anderen Stelle suchen
bzw. diese Situation im Programm abfangen.
Ansonsten natürlich vielen Dank für die Nachhilfe zum Thema "dynamische
Listen", ich werde die Hinweise in Zukunft beachten. Und vielleicht sind
die Ausführungen von KH auch für andere lehrreich....
Grüße Uwe