[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?

Nachdem die Arbeiten bei Live derzeit etwas abklingen, finde ich mal wieder ein bisschen Zeit, mit EPGsearch weiterzumachen. Der nächste Schritt wäre, die Strings anstelle in Puffern fester Länge (char[MaxFileName]) in STL-Strings abzuspeichern, die ja auch schon an vielen anderen Stellen im Code genutzt werden. Und die diversen Parser sollen auch über eine Parser-Klasse abstrahiert werden. Einen Prototyp auf Funktionsbasis hatte ich ja schon gepostet. Ein einer Parser-Klasse erfolgt die Wertezuweisung dann über einen virtuellen Value-Handler.

Bei den ersten Schritten ist mir aufgefallen, dass im Code etliche using-Direktiven enthalten sind, damit man sich das Namensraum-Präfix std spart. Leider wird das auch in Header-Dateien vielfach genutzt, was aber prinzipiell vermieden werden soll (siehe etwas hier), weil es ansonsten zu Problemen mit konkurrierenden using-Direktiven in anderen Headern kommen kann.

Ähnlich wie dies bspw. auch schon in Live erfolgt ist, habe ich in Vorbereitung des nächsten Schritts deshalb alle using-Direktiven entfernt und die verwendeten Datentypen mit dem Namensraum-Präfix ausgestattet:

• 0001 Refactored using directives into namespace prefixes.zip

Der Code wirkt dadurch auf den ersten Blick zwar zunächst vielleicht etwas ungewohnt, aber man gewöhnt sich, wie ich bei Live festgestellt habe, recht schnell daran. Zumal wurde dies in EPGsearch bislang auch nicht einheitlich gehandhabt.

TomJoad, magst du dir das mal ansehen und entscheiden, ob das in die Codebasis einfließen kann? Ich wäre dafür, um dann auf dieser Basis die nächsten Schritte anzugehen.

PS: Weil das nur syntaktische Änderungen sind, auf die der Compiler ein wachsames Auge hat, funktioniert das Plugin bei mir damit unverändert.

Quote from TomJoad

Die using-Geschichte ist im git.

Danke, dann arbeite ich auf dieser Basis weiter.

Quote from TomJoad

Bleiben nur die Contentsfilter. Ich würde sie wie search und directory löschen. Wenn sie erhalten bleiben sollen, muss halt der EDIS-String lang genug sein.

Das habe ich bereits schon so geändert, damit das Verhalten dieser drei Parameter konsistent ist. Für alle numerischen Parameter habe ich Bereichsprüfungen vorgesehen, sodass sich über das Service-Interface keine ungültigen Daten einschleichen können. Klare Fehlermeldungen vereinfachen auch die Integration von EPGsearch in Plugins, die das Service-Interface nutzen möchten.

Derzeit werden auf die Contents-Filter noch die Escape-Squenzen für Doppelpunkte und Pipe-Zeichen angewandt. Aber da die Contents-Filter ja immer nur Pärchen von Hex-Ziffern ohne Trennzeichen sind, braucht man diese nicht, weshalb ich sie entsorgt habe.

Quote from TomJoad

Was mir noch Bauchschmerzen in dem Zusammenhang bereitet, dass man mit falschen Kanaleinträgen z.B. den vdr zum Absturz bringen kann.

Ich behalte das mal im Hinterkopf. Vielleicht fällt mir diesbezüglich etwas auf. Passiert das schon beim Zuspielen über das Service-Interface oder erst beim Ausführen einer Suche?

Ohne die vielen geänderten Zeilen aufgrund der unterschiedlichen Einrückungen sieht das Ganze eigentlich ganz übersichtlich aus. Puh, Glück gehabt…

Deine Änderungen habe ich mit einfließen lassen. Auch wenn die technische Umsetzung im neuen Parser anders aussehen wird, ist die Semantik die gleiche: Ein Parserfehler deaktiviert den Timer, und SVDRP reagiert entsprechend deiner Ergänzung auf den Fehlerstatus.

Quote from TomJoad

Mein Vorschlag für den nächsten Stand wäre der aus dem Anhang

Der Patch hat sich bei mir im Produktiveinsatz unauffällig verhalten und löscht die Contents-Filter wie gedacht.

Kannst du ihn bitte zeitnah ins Git bringen (oder wenigstens Zeile 412 zum Löschen der Filter)? Ich bereite für MarkusE gerade einen Patch vor, der in Live bislang fehlende Parameter von Suchtimern ergänzt – unter anderem die Contents-Filter. Und wenn man die dann nicht löschen könnte, wäre das schon ziemlich unangenehm.

Wie wäre es, diesem Stand die Versionsnummer 2.4.4 zu geben, damit wir diesen für die Contents-Filter essentiellen Fix klar adressieren können: "Bitte auf Version 2.4.4 upgraden…"

Noch etwas: Wenn ich folgende Inhaltsfilter setze:

… werden Sendungen, die nur eine der beiden Kennungen enthalten, nicht gefunden.

Ich würde intuitiv erwarten, dass innerhalb einer Gruppe jede Sendung gefunden wird, die mindestens eines der Filterkriterien enthält. Denn wie soll ich sonst nach Sendungen suchen, die entweder ein Krimi (Detektiv/Thriller) oder ein Actionfilm (Abenteuer/Western/Krieg) sind?

Anders gesagt: Innerhalb der Gruppen würde ich eine Oder-Verknüpfung, zwischen den Gruppen eine Und-Verknüpfung erwarten.

Derzeit ist es aber offensichtlich eine Und-Verknüpfung jeder gewählten Inhaltskategorie, denn in Zeile 18:

bool cSearchExt::MatchesContentsFilter(const cEvent* e)
{
    if (!e) return false;
    // check if each content filter ID is contained in the events descriptors
    for (unsigned int i = 0; i < contentsFilter.size(); i += 2) {
        std::string hexContentID = contentsFilter.substr(i, 2);
        if (hexContentID.size() != 2) return false;
        std::istringstream iss(hexContentID);
        int searchContentID = 0;
        if (!(iss >> std::hex >> searchContentID)) return false;
        int c = 0, eventContentID = 0;
        bool found = false;
        while ((eventContentID = e->Contents(c++)) > 0)
            if (eventContentID == searchContentID) {
                found = true;
                break;
            }
        if (!found) return false;
    }
    return true;
}

… wird sofort abgebrochen, wenn eine der Inhaltskategorien nicht gefunden wird.

Deshalb die Frage, wie die Inhaltsfilter eigentlich gedacht sind.