[epgsearch] Kleinigkeiten, die aufgefallen sind

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

    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:

    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.

    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… ;)

    Hardware: Antec NSK2480, Asus P8B75-M LX, Intel Core i5-3570T, 4 GB RAM, NVIDIA GT610, TT-Premium S2-6400, 128 GB SSD, 14 TB HDD, Pioneer BDR-207EBK
    Software: Ubuntu 22.04 LTS mit Kernel 6.8 und VDR 2.7.5 (mit offiziellen und eigenen Patches)
    Plugins: devstatus, dvbhddevice, dvd, dvdswitch, epgsearch, extrecmenu, recsearch, femon, live, markad, mlist, osdteletext, remote, screenshot, skinnopacity, streamdev, systeminfo, undelete, xineliboutput
    Addons: VDR Convert 0.1.0 (angepasst)

    Edited 2 times, last by SHofmann (May 29, 2025 at 8:31 PM).

  • 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:

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

    Hardware: Antec NSK2480, Asus P8B75-M LX, Intel Core i5-3570T, 4 GB RAM, NVIDIA GT610, TT-Premium S2-6400, 128 GB SSD, 14 TB HDD, Pioneer BDR-207EBK
    Software: Ubuntu 22.04 LTS mit Kernel 6.8 und VDR 2.7.5 (mit offiziellen und eigenen Patches)
    Plugins: devstatus, dvbhddevice, dvd, dvdswitch, epgsearch, extrecmenu, recsearch, femon, live, markad, mlist, osdteletext, remote, screenshot, skinnopacity, streamdev, systeminfo, undelete, xineliboutput
    Addons: VDR Convert 0.1.0 (angepasst)

  • 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 bin zu diesem ganzen Code gekommen, weil ich mit anderen die Anpassungen an die neue Lockstruktur gemacht habe. Ich nutze selber längst nicht alle Möglichkeiten von epgsearch und kann auch nicht behaupten, daß ich den Code im Detail durchschaue. Seither habe ich mich bemüht, notwendige Anpassungen zu machen und offensichtliche Fehler zu beheben. Ich freue mich über jede Zuarbeit. Nur wenn das API oder der Menüaufbau irgendwie geändert wird, sollte es im Forum breiter diskutiert werden.

    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.

    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.

    vdr-2.7.5

    softhddevice, dbus2vdr, dvd, epgsearch, femon, graphtftng, web, menuorg,
    osdteletext, radio, recsearch, satip, tvguide, vnsiserver
    ubuntu focal, yavdr-ansible, linux-6, AsRock J4105, CIne CT-V7 DVB-C

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

    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.

    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:

    Code
    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…

    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… :/

    Hardware: Antec NSK2480, Asus P8B75-M LX, Intel Core i5-3570T, 4 GB RAM, NVIDIA GT610, TT-Premium S2-6400, 128 GB SSD, 14 TB HDD, Pioneer BDR-207EBK
    Software: Ubuntu 22.04 LTS mit Kernel 6.8 und VDR 2.7.5 (mit offiziellen und eigenen Patches)
    Plugins: devstatus, dvbhddevice, dvd, dvdswitch, epgsearch, extrecmenu, recsearch, femon, live, markad, mlist, osdteletext, remote, screenshot, skinnopacity, streamdev, systeminfo, undelete, xineliboutput
    Addons: VDR Convert 0.1.0 (angepasst)

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

    Da wäre ich eher für die Umstellung auf die Klassen cString und cStringList aus VDR tools.c. Ich habe beide bei meinem Plugins schätzen gelernt und die Syntax ist IMHO einfacher:
    Anstatt z.B. str.c_str() schreibt man einfach *str und bekommt einen const char * zurück

  • 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:

    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.

    Hardware: Antec NSK2480, Asus P8B75-M LX, Intel Core i5-3570T, 4 GB RAM, NVIDIA GT610, TT-Premium S2-6400, 128 GB SSD, 14 TB HDD, Pioneer BDR-207EBK
    Software: Ubuntu 22.04 LTS mit Kernel 6.8 und VDR 2.7.5 (mit offiziellen und eigenen Patches)
    Plugins: devstatus, dvbhddevice, dvd, dvdswitch, epgsearch, extrecmenu, recsearch, femon, live, markad, mlist, osdteletext, remote, screenshot, skinnopacity, streamdev, systeminfo, undelete, xineliboutput
    Addons: VDR Convert 0.1.0 (angepasst)

  • Quote

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

    Mit malloc / realloc / free hat man bei cString nichts mehr zu tun - das erledigt alles die Klasse, auch einem cString einen anderen zuweisen geht problemlos (ohne explizites strdup()), ebenso wie ein char* einem cString zuweisen. Und zurück zu const char* gehts einfach mit *cString wie schon geschrieben. Schau Dir mal z.B. mein skinElchiHD an oder im VDR selbst.

  • 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:

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

    Hardware: Antec NSK2480, Asus P8B75-M LX, Intel Core i5-3570T, 4 GB RAM, NVIDIA GT610, TT-Premium S2-6400, 128 GB SSD, 14 TB HDD, Pioneer BDR-207EBK
    Software: Ubuntu 22.04 LTS mit Kernel 6.8 und VDR 2.7.5 (mit offiziellen und eigenen Patches)
    Plugins: devstatus, dvbhddevice, dvd, dvdswitch, epgsearch, extrecmenu, recsearch, femon, live, markad, mlist, osdteletext, remote, screenshot, skinnopacity, streamdev, systeminfo, undelete, xineliboutput
    Addons: VDR Convert 0.1.0 (angepasst)

  • Wenn öfter mal substrings benötigt werden, dann sollte aus Performancegründen std::string_view verwendet werden. std::string_view erlaubt substrings ohne malloc, und ist daher deutlich performanter als die Erzeugung von substrings mit std::string/cString.

    Voraussetzung von std::string_view ist ein gcc compiler der -std=c++17oder -std=c++1z unterstützt. Sollte heute kein Problem mehr sein, das live plugin hat diese Voraussetzung auch.

  • Ich komme erst Pfingsten wieder an meine Entwicklungsumgebung. Wo sich jetzt doch einiges tut, finde ich es eine gute Idee, einen Testbranch dann im git einzurichten

    vdr-2.7.5

    softhddevice, dbus2vdr, dvd, epgsearch, femon, graphtftng, web, menuorg,
    osdteletext, radio, recsearch, satip, tvguide, vnsiserver
    ubuntu focal, yavdr-ansible, linux-6, AsRock J4105, CIne CT-V7 DVB-C

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

    Hardware: Antec NSK2480, Asus P8B75-M LX, Intel Core i5-3570T, 4 GB RAM, NVIDIA GT610, TT-Premium S2-6400, 128 GB SSD, 14 TB HDD, Pioneer BDR-207EBK
    Software: Ubuntu 22.04 LTS mit Kernel 6.8 und VDR 2.7.5 (mit offiziellen und eigenen Patches)
    Plugins: devstatus, dvbhddevice, dvd, dvdswitch, epgsearch, extrecmenu, recsearch, femon, live, markad, mlist, osdteletext, remote, screenshot, skinnopacity, streamdev, systeminfo, undelete, xineliboutput
    Addons: VDR Convert 0.1.0 (angepasst)

    Edited once, last by SHofmann (June 1, 2025 at 1:26 PM).

Participate now!

Don’t have an account yet? Register yourself now and be a part of our community!