[epgsearch] Kleinigkeiten, die aufgefallen sind

Quote from TomJoad

Ich würde den Parameter weiterhin bei 1 beginnen lassen, damit die Nummerierung mit der Ausgabe von man epgsearch.conf zusammenpasst.

Gut, dann lassen wir das so. Andernfalls hätte ich man epgsearch.conf halt entsprechend angepasst.

Quote from TomJoad

Gibt es einen Grund, dass das Zurückschreiben von leeren Feldern (::) den alten Wert bestehen lassen soll oder war das ein Bug?

Der derzeitige Codes führt genau zu diesem Verhalten:

    while (*pos) {
        while (*pos == ' ') pos++;
        if (*pos) {
            if (*pos != ':') {
                ...
                pos = pos_next;
                switch (parameter) {
                case 1:
                    ...
                default:
                    break;
                } //switch
            }
            parameter++;
        }
        if (*pos) pos++;
    } //while

Wenn nach einem Doppelpunkt unmittelbar ein weiterer folgt (führende Spaces werden in Zeile 2 übersprungen), dann wird switch nicht durchlaufen und das entsprechende Attribute nicht somit auch nicht modifiziert. Insofern würde ich dies eher als – durchaus nützliches – Feature betrachten. Ein paar Kommentare in diese Richtung gab es weiter oben ja schon.

Quote from TomJoad

In welchem Zusammenhang kommt ... vor?

Das war nur aus editorischen Gründen, damit ich nicht den ganzen Kram davor in den Post kopieren musste.

Noch ein paar ergänzende Infos… Die Arbeiten am Code fallen wohl doch etwas umfangreicher aus:

• Ich habe auf Anregungen von SHF schon mal alle Patterns der Form if (p) free(p); auf free(p); reduziert, weil free(NULL); zulässig ist und intern das gleiche macht. Damit wird der Code leichter lesbar.
• Die diversen strreplace-Funtionen habe ich etwas gehärtet, damit bspw. bei einem leeren Suchstring keine Endlosschleife entsteht. Klaus zieht das im VDR ebenfalls nach.
• Ein paar Memory Leaks, die mir aufgefallen sind, habe ich ebenfalls bereinigt.
• Die Parser habe ich mit Checks ausgestattet, die bei Parse-Fehlern oder unzulässigen Parameterwerten einen aussagekräftigen Syslog-Eintrag erzeugen. Ob man nach einem Validierungsfehler weitere Parameter einlesen möchte, lässt sich per Flag steuern (default: wie bisher). Die Idee ist, beim Laden der Konfiguration fehlertolerant zu sein, damit EPGsearch so gut wie möglich losläuft, beim SVDRP-Interface und API (also beim Neuanlegen und Bearbeiten von Objekten) aber streng, damit keine fehlerhaften Daten in die Datenbasis einfließen.
• Die etwas unübersichtlichen Schleifen der Form cSearchExtCat *SearchExtCat = SearchExtCats.First();int index = 0; while (SearchExtCat) { ...;         SearchExtCat = SearchExtCats.Next(SearchExtCat); index++; } habe ich dort, wo SearchExtCat im Schleifenrumpf nicht für andere Objektzugriffe genutzt wird – also insbesondere bei der Verwaltung der catvalues – auf for (int index = 0; index < SearchExtCats.Count(); index++) { ... } reduziert. Weil etwa die catvalues auch anhand von SearchExtCats.Count() dimensioniert werden, ist das nicht nur schlüssiger, sondern zudem auch lesbarer.
• Die Puffer fester Größe sind ein echtes Problem. Beispielsweise fassen sie nicht einmal alle Genres aus dem EPG-Sample:
  
  [BASH] s="Abenteuer,Action,Boxen,Comedy,Dokumentarfilm,Drama,Erotik,Familien-Show,Fantasy,Fussball,Geschichte,Gesellschaft,Gesundheit,Gymnastik,Handball,Heimat,Humor,Jazz,Kinderfilme,Kindernachrichten,Kinderserien,Klassik,Krankenhaus,Krimi,Kultur,Kurzfilm,Motor+Verkehr,Motorsport,Musik,Mystery,Nachrichten,Natur,Politik,Radsport,Ratgeber,Reise,Rock,Romantik/Liebe,Science Fiction,Soap,Spielshows,Talkshows,Tennis,Thriller,Verschiedenes,Volksmusik,Wassersport,Western,Wintersport,Wirtschaft,Wissen,Zeichentrick"
  
  [BASH] echo ${#s}
  500
  
  [BASH] printf "%.*s" 254 "$s"
  Abenteuer,Action,Boxen,Comedy,Dokumentarfilm,Drama,Erotik,Familien-Show,Fantasy,Fussball,Geschichte,Gesellschaft,Gesundheit,Gymnastik,Handball,Heimat,Humor,Jazz,Kinderfilme,Kindernachrichten,Kinderserien,Klassik,Krankenhaus,Krimi,Kultur,Kurzfilm,Motor+Ve
  
  Und wenn man im OSD alle diese Einträge aktiviert, stürzt der VDR noch im OSD ab. Ich beabsichtige deshalb, die ganzen Puffer fester Größe zu eliminieren und mich eher an cReadLine des VDR zu orientieren, sprich: getline() mit dynamisch allokiertem Puffer zu nutzen und auch in den Objekten char value[MaxFilename] durch char* value usw. zu ersetzen. Oder doch gleich besser std::string?
• Alle Aufrufe von strdup() usw. sollte man auf Fehlerzustände (NULL-Pointer) prüfen und eine sinnvolle Fehlerbehandlung vorsehen. Das wäre aber wohl eher ein getrennter Schritt, sprich: Pull-Request.
• Auch sollte man die bislang öffentlichen Klassen-Attribute verbergen und den Zugriff – wie etwa im VDR – ausschließlich über Getter und Setter zulassen. Dann wäre es auch egal, ob String-Attribute intern als char[], char* oder std::string repräsentiert würden. Ebenfalls etwas für die To-Do-Liste.

Etliche Tage an intensiver Arbeit sind da schon reingeflossen. Man stolpert von einem unschönen Effekt zum Nächsten, deshalb auch die inzwischen schon etwas umfangreiche Liste von oben. Ein gründlicher Review samt Testphase sind dann unbedingt notwendig.

Aber wenn dir das alles nicht recht ist, wäre es fair, das gleich zu sagen, bevor ich noch mehr Zeit investiere und du den Pull-Request am Schluss doch nicht nimmst.

Und eigentlich wollte ich für Suchtimer ja nur ein weiteres Feld für Notizen haben…

Ich hätte noch ein Argument, die Zählung zumindest bei manchen Parsern ab "0" zu starten. Bei Suchtimern, Ausschlusslisten usw. ist der erste "Parameter" ja stets ein Objekt-Identifikator, und erst danach folgen die eigentlichen Attribute für das entsprechende Objekt. Wenn also ein Datensatz mit einem Objekt-Identifikator beginnt, wäre es dann nicht vielleicht doch angebracht, diesen als "0" zu führen und die eigentlich Parameter dann ab "1" beginnend?

Hier das Beispiel eines Suchtimer-Objekts:

switch (parameter) {
	// search to be processed
	case 0:	// the identifier
        ...
		break;
   	// search parameters 1..n
    case 1: // an attribute of the search
    	...

Aber wie gesagt, ich kann das auch wieder rückgängig machen.

Danke, das freut mich zu hören. Dann werde ich an meinen Anpassungen weiterarbeiten und zum Review stellen, wenn ich sie für "gereift" halte.

Quote from TomJoad

Die Zählung ist wirklich Geschmackssache (außer es handelt sich um Arrays gleichen Datentyps). Da die man-Page sowieso mal ergänzt werden muss, kann man auch mit 0 beginnen. Der Mensch ist nur halt kein Computer.

Ich werde es erst einmal so lassen, wie im Beitrag davor beschrieben: Wenn das erste Feld eine ID ist, beginnt die Zählung mit 0, sonst mit 1. Damit sind alle "echten" Parameter ab 1 zu finden. Bei Nichtgefallen können wir das immer noch zurückdrehen, bevor der Code vom Test- auf den Master-Branch geht.

Quote from TomJoad

Memory Leaks sollten auf jeden Fall bereinigt werden, ob man bei einer Out-of-Memory-Condition noch irgendwie sinnvoller reagieren kann als mit einem zu erwartenden Crash, mag ich nicht beurteilen.

Zumindest sollte man einen entsprechenden Eintrag ins Syslog schreiben, damit man die Ursache kennt und die auslösende Stelle schnell identifizieren kann, ohne erst das ganze Coredump-Environment zum Laufen bringen zu müssen. Das könnte etwa wie folgt aussehen:

if (!s) {
	LogFile.eSysLog("ERROR file \"%s\", line %d: memory allocation failed", __FILE__, __LINE__);
	// some rectifications
    return false;
}

Der Aufrufer sollte mit der Rückgabe von false natürlich möglichst sinnvoll umgehen. Ob das den Crash verhindert, ist natürlich weiterhin fraglich…

Quote from TomJoad

Dass im Code bisher leere Felder beim Editieren übersprungen wurden, halte ich für eine Nachlässigkeit, die bisher nicht gestört hat. In der Regel waren leere Felder ja auch vorher leer. Es wäre jedenfalls intuitiver, ohne Einfügen eines Blanks zu löschen.

Ich würde das bisherige Verhalten nur ungern ändern, weil es durchaus Nutzer geben mag, die sich bei ihrem Skripting darauf verlassen. Für den Contents-Filter bietet sich an, wie für den Suchstring und das Serien-Verzeichnis den String vor dem Parsen explizit zu löschen, sodass er immer wieder aufs neue mitgeschickt werden muss. Das wäre zwar anders als bei numerischen Feldern, die ja auch nicht auf ihren Defaultwert zurückspringen, aber zumindest konsistent für alle "reinen" String-Parameter ohne eigenen Schalter bzw. Mode-Auswahl.

Was die Begrenzung der Stringlängen betrifft, habe ich mal ein wenig herumgespielt:

• Naheliegenderweise könnte man natürlich bei char* bleiben und Strings (um unnötige free()-strdup()-Kaskaden und infolgedessen Heap-Fragmentierung zu vermeiden) diese bei Zuweisung per realloc() in der Länge anpassen (möglichst nur anwachsen lassen). Im Kontext von C++ fände ich das aber etwas altbacken.
• Von Vorteil wäre wohl eher, von char[] und char* auf std::string bzw. von char** auf std::vector<std::string> und ab da vielleicht sukzessive auf die std-Bibliothek umzustellen. Mit std::string_view könnte man – C++17 als minimaler Standard seit GCC v11 vorausgesetzt – dann innerhalb der Parser auch effizient auf Teilstrings arbeiten.

Hast du diesbezüglich eine Präferenz?

Auch ist die Namensgebung von Variablen und Attributen uneinheitlich. Wenn man bei der Refaktorisierung auf die std-Bibliothek umstellt, sollte man alle Attribute als private deklarieren (Schreibweise: camelCase) und über Getter/Setter (Schreibweise: Get/SetCamelCase()) arbeiten. Im Fall von (2) sollte man das gleich miterledigen. Wird dann aber ein ziemlich umfangreiches Patchset…

Ein guter Hinweis. Der Nachteil ist IHMO aber, dass diese noch immer char* sind und damit all deren inhärente Probleme und Nachteile aufweisen – insbesondere den ganzen Zirkus mit strn0cpy(), strdup(), malloc() / realloc() und free().

Ich dagegen habe bei Live die String-Verarbeitung per std::string_view zu schätzen gelernt. Diese lässt sich sowohl auf char* als auch auf std::string anwenden. Markus hat das sogar in einer ganzen Sammlung von Helper-Klassen (stringhelpers.h) nochmals ein wenig komfortabler zu handhaben gemacht. Allerdings ist das schon sehr fortgeschrittenes Handtieren mit Template-Klassen, weshalb ich auf dieses Bibliothek lieber nicht zurückgreifen möchte, damit die Einstiegshürde für EPGsearch auf dem "Standard-C++"-Level bleibt. Denn dafür findet man bspw. bei Stack Overflow fast immer Antworten auf Fragen.

So unterschiedlich sind halt die Erfahrungen. Anbei mal ein Prototyp für einen Parser auf Basis von std::string_view:

• Sample.cpp.txt

Kein Zwang mehr, Substrings immer mit einen NUL-Character versehen oder Segmente per Boundary-Variablen führen zu müssen, und auch kein Beachten von Null-Pointern. Das sind für mich die überzeugenderen Argumente, weil damit viele klassische Pointer-Fehler bei der String-Verarbeitung a priori vermieden werden.

Natürlich kann man die Strings in den Objekten weiterhin als char* speichern, muss sich dann aber doch wieder mit realloc() oder Heap-Fragmentierung auseinandersetzen. Insofern erachte ich die Zeit, den Code von char* auf cString (unter der Haube ja noch immer char*) zu überarbeiten, schlecht investiert und werde das deshalb auch nicht angehen. Also std::string_view zum Parsen und der Rest bleibt (fast) wie er ist, oder konsequent std::string.

Ich habe mir cString natürlich schon vor einiger Zeit angeschaut. Schon deshalb, weil man im (Kontext des) VDR gar nicht darum herumkommt. Dennoch finde ich, man sollte den Sprachstandard bzw. die zugehörige Bibliothek verwenden, wenn dies das Gewünschte leistet. Klaus hatte seinerzeit sicherlich gute Gründe, cString einzuführen, aber für EPGsearch sehe ich diese bisher nicht.

Und selbst cString hat noch Fehler:

cString &cString::Append(const char *String)
{
  if (String) {
     int l1 = s ? strlen(s) : 0;
     int l2 = strlen(String);
     if (char *p = (char *)realloc(s, l1 + l2 + 1)) {
        s = p;
        strcpy(s + l1, String);
        }
     else
        esyslog("ERROR: out of memory");
     }
  return *this;
}

Denn wenn !(p = realloc(...)), dann weiß man als Aufrufer nicht, was man für den String zurückbekommen hat…

Wie gestaltet sich dann der Ablauf? Trägst du mich als Contributor ein, sodass ich direkt auf den Test-Branch pushen kann? Oder soll ich einen Pull-Request von meinem Fork aus stellen?