Ab und zu Programmabsturz, wahrscheinlich wg. verketteter Liste

[Uwe B. (boerge)]

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:

struct playlist_struct {
  char name[30];
  struct playlist_struct* prev;
  struct playlist_struct* next;
};

struct playlist_struct* playlists = NULL;


//**********************************************************************
void mpd_lsplaylists(MpdObj *mi) 
{
    MpdData *data;
    struct playlist_struct* ptr;
    struct playlist_struct* prev;
    
    // eventuell alte vorhandene Liste mit Playlisten loeschen
    if (playlists != NULL) {
        // an Anfang der Liste...
        while (playlists->prev!=NULL) playlists=playlists->prev;
        // ...und bis zum Ende loeschen
        while (playlists != NULL) {
            ptr = playlists->next;
            free(playlists);
            playlists = ptr;  
        }
    }  
    // Liste mit Playlisten erzeugen...
    for (data=mpd_database_playlist_list(mi); data!=NULL; data=mpd_data_get_next(data)){
        if (playlists == NULL) {
            playlists = malloc(sizeof(*playlists));  
            strcpy(playlists->name, data->playlist->path);
            playlists->prev = NULL;
            playlists->next = NULL;
        } else {
            ptr = playlists;
            while (ptr->next != NULL) ptr = ptr->next;
            prev = ptr;
            ptr->next = malloc(sizeof(*ptr));  
            ptr=ptr->next;
            strcpy(ptr->name, data->playlist->path);
            ptr->prev = prev;
            ptr->next = NULL;
        }  
    }
}

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...):

// ********************************************
void mpd_queue_output(void)
{
    int y = 30;
  
    if (playlists != NULL) {
        put_string(10, y, "Playlists:", GREY, FONT_8X8);
        y+=20;
        put_string(10, y, playlists->name, LIGHT_GREY, FONT_8X16);
    } else {
        put_string_center(160, 80, "No playlists found!", LIGHT_GREY, FONT_8X16);
    }
}

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.

[Sven (Gast)]

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

[Uwe B. (boerge)]

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

[Karl H. (kbuchegg)]

            strcpy(playlists->name, data->playlist->path);

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.

[Gregor O. (zappes)]

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.)

[Karl H. (kbuchegg)]

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.

void deletePlaylists()
{
  struct playlist_struct* pNext;

  while (playlists != NULL) {
    pNext = playlists->next;
    free( playlists );
    playlists = pNext; 
  }
}

void mpd_lsplaylists(MpdObj *mi) 
{
    MpdData *data;
    struct playlist_struct* newItem;
    struct playlist_struct* prevItem;
  
    deletePlaylists();
 
    for ( data = mpd_database_playlist_list(mi);
          data != NULL;
          data = mpd_data_get_next( data ) ) {

      // neuen Listeneintrag erzeugen
      newItem = malloc( sizeof(*newItem) );

      strncpy( newItem->name, data->playlist->path, sizeof( newItem->name ) );
      newItem->name[ sizeof( newItem->name ) - 1 ] = '\0';   // nur um sicher zu gehen
      newItem->next = NULL;
      newItem->prev = NULL;

      // und einhaengen
      if( playlists == NULL )
        playlist = newItem;
      else {
        prevItem->next = newItem;
        newItem->prev = prevItem;
      }

      // das jetzige Item wird im naechsten Schleifendurchlauf
      // zum vorhergehenden Item des naechsten zu erzeugenden Items
      prevItem = newItem;
    }
}

[Karl H. (kbuchegg)]

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.

void dumpPlaylist()
{
  struct playlist_struct* pPtr = playlists;

  printf( "Dump of Playlists:\n" );
  while (pPtr) {
    printf( "  %s\n", pPtr->name );
    pPtr = pPtr->next;
  }
}

[Dennis (Gast)]

Kann es sein, dass beim malloc (sizeof(*playlist)) der Stern weg muss?

[Karl H. (kbuchegg)]

Dennis schrieb:
> Kann es sein, dass beim malloc (sizeof(*playlist)) der Stern weg muss?

Nein

[Karl H. (kbuchegg)]

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.

void deletePlaylists()
{
  struct playlist_struct* pNext;

  while (playlists != NULL) {
    pNext = playlists->next;
    free( playlists );
    playlists = pNext; 
  }
}

struct playlist_struct* createPlaylistItem( const char* name )
{
  struct playlist_struct* pNew;

  pNew = malloc( sizeof(*pNew) );
  if( pNew ) {
    strncpy( pNew->name, name, sizeof( pNew->name ) );
    pNew->name[ sizeof( pNew->name ) - 1 ] = '\0';   // nur um sicher zu gehen
    pNew->next = NULL;
    pNew->prev = NULL;
  }

  return pNew;
}

void mpd_lsplaylists(MpdObj *mi) 
{
  MpdData *data;
  struct playlist_struct* newItem;
  struct playlist_struct* prevItem;
  
  deletePlaylists();
 
  for ( data = mpd_database_playlist_list(mi);
        data != NULL;
        data = mpd_data_get_next( data ) ) {

    // neuen Listeneintrag erzeugen
    newItem = createPlaylistItem( data->playlist->path );

    // und einhaengen
    if( newItem ) {
      if( playlists == NULL )
        playlist = newItem;
      else {
        prevItem->next = newItem;
        newItem->prev = prevItem;
      }

      // das jetzige Item wird im naechsten Schleifendurchlauf
      // zum vorhergehenden Item des naechsten zu erzeugenden Items
      prevItem = newItem;
    }
  }
}

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.

struct playlist_struct* playlists = NULL;
struct playlist_struct* currentPlaylistItem = NULL;

[Uwe B. (boerge)]

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:

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