Compile Warnings in vnsiserver

Quote from SHF

Verwendet jemand zufällig das SAT>IP-Plugin in Verbindung mit ShowChannelNamesWithSource == 2 (Das ist mit Anzeige der SAT-Position.)?

Nur um sicher zu stellen, dass es nicht daran liegt.

Zum Testen sollte es reichen das zu aktivieren und die ein- zwei-mal die Kanalliste durch zu scrollen.

Mit einer DVB-Karte werde ich es die Tage mal testen, aber mit SAT>IP kann ich es leider nicht.

Ich habe das eben mal eingestellt, habe aber nur 56 Kanäle in der channels.conf.

Ich konnte jedoch keinen crash provozieren.

Wenn es satip ist, dann müsste die sources.conf editiert worden sein, um die Auswahl des Satelliten festzulegen. Wäre also auch mal interessant, die Datei zu sehen.

Aber da wir immer über cChannel::Name stolpern:

Was ich auch nicht so ganz verstehe, ist die Bedeutung der Variable nameSourceMode in vdr/channels.c.

Diese Variable wird niemals initialisiert im Konstruktor, aber wird direkt in cChannel::Name verwendet. Ich hätte im Konstruktor erwartet

nameSourceMode = Setup.ShowChannelNamesWithSource;

Wenn nameSourceMode zugewiesen wird, wird immer der gleiche Wert '0' zugewiesen.

Diese Variable ist also entweder uninitiatlisiert, oder gleich '0'. Kein anderer Wert.

Ich hätte ja auch erwartet, dass diese Variable dann nach diesem Vergleich in channels.c:110 neu gesetzt wird:

     if (isempty(nameSource) || nameSourceMode != Setup.ShowChannelNamesWithSource) {
++      nameSourceMode = Setup.ShowChannelNamesWithSource;
        if (Setup.ShowChannelNamesWithSource == 1)
           nameSource = cString::sprintf("%s (%c)", name, cSource::ToChar(source));
        else
           nameSource = cString::sprintf("%s (%s)", name, *cSource::ToString(source));
        }

Auch in cChannel::operator= wird der variable 0 zugewiesen, anstelle vom rhs Channel.nameSourceMode

In cChannel::SetTransponderData wird dann nameSourceMode auf 0 gesetzt (warum nicht Setup.ShowChannelNamesWithSource?), wenn

if (source != Source || frequency != Frequency || srate != Srate || strcmp(parameters, Parameters)) {

Dabei lohnt sich das nur, wenn source != Source.. Jede einzelne Änderung an den Transponderdaten bewirkt Neuzweisung von nameSource, obwohl das gar nicht nötig wäre.

Hat jemand ne Idee?

Quote from wirbel

Jede einzelne Änderung an den Transponderdaten bewirkt Neuzweisung von nameSource, obwohl das gar nicht nötig wäre.

Hat jemand ne Idee?

Der Knackpunkt müsste aber sein, dass wir was am cCannel-Objekt ändern, obwohl wir nur einen readlock darauf haben.

Egal ob mit oder ohne den Patch, die einfache Abfrage von cChannel::Name() ändert in jedem Fall cChannel::nameSource und ist damit immer ein Schreibzugriff.

Und noch schlimmer - das bei jedem Zugriff auf cChannel::Name(), da die if Bedingung

if (isempty(nameSource) || nameSourceMode != Setup.ShowChannelNamesWithSource) {

niemals falsch ergeben kann. nameSourceMode wird immer '0' bleiben und Setup.ShowChannelNamesWithSource bleibt immer bei dem was der Nutzer einstellt, also hier '2'.

Es fehlt also in jedem Fall ein Lock oder ähnliches. Aber dafür müsste nur cChannel gelockt werden, nicht die ganze Kanalliste.

Ich denke mal, dass das hier fehlt:

--- channels.c  2021/05/21 09:38:34     5.1
+++ channels.c  2024/03/01 07:22:19
@@ -112,6 +112,7 @@
            nameSource = cString::sprintf("%s (%c)", name, cSource::ToChar(source));
         else
            nameSource = cString::sprintf("%s (%s)", name, *cSource::ToString(source));
+        nameSourceMode = Setup.ShowChannelNamesWithSource;
         }
      return nameSource;
      }

Wird zusätzlich ein Lock benötigt? Verändert ja cChannel, evtl. von mehreren Threads aus.

Vielleicht so (ungetestet!):

--- channels.c  2021/05/21 09:38:34     5.1
+++ channels.c  2024/03/01 07:42:18
@@ -106,12 +106,15 @@

 const char *cChannel::Name(void) const
 {
+  static cMutex mutex;
   if (Setup.ShowChannelNamesWithSource && !groupSep) {
+     cMutexLock MutexLock(&mutex);
      if (isempty(nameSource) || nameSourceMode != Setup.ShowChannelNamesWithSource) {
         if (Setup.ShowChannelNamesWithSource == 1)
            nameSource = cString::sprintf("%s (%c)", name, cSource::ToChar(source));
         else
            nameSource = cString::sprintf("%s (%s)", name, *cSource::ToString(source));
+        nameSourceMode = Setup.ShowChannelNamesWithSource;
         }
      return nameSource;
      }

RHS Wenn ShowChannelNamesWithSource == 0 ist, dann kommt er an die Stelle gar nicht hin.

Gut wäre diese Zeile in vdr/channels.c:56ff noch hinzuzufügen.

Ansonsten hat die Variable u.U. einen beliebigen Startwert.

cChannel::cChannel(void)
{
  name = strdup("");
  shortName = strdup("");
  provider = strdup("");
  portalName = strdup("");
  memset(&__BeginData__, 0, (char *)&__EndData__ - (char *)&__BeginData__);
  parameters = "";
++nameSourceMode = 0;
  modification = CHANNELMOD_NONE;
  seen         = 0;
  schedule     = NULL;
  linkChannels = NULL;
  refChannel   = NULL;
}

RHS Ich verfolge diesen Thread nur so am Rande.

Hallo RHS,

ja, das macht definitiv Sinn, Klarheit über die Teststände ist essentiell.

Aber ohne deine intensiven Tests gäbe es keinen Fortschritt bei vnsi, danke dafür.

Ich packe das mal in zwei temporäre github repos auf meinem Account, das macht es leichter für dich.

------------------------------------------------------------------------------------

für vnsiserver bitte das hier verwenden:

GitHub - wirbel-at-vdr-portal/vdr-plugin-vnsiserver-crashes: VDR plugin to handle XBMC clients.

VDR plugin to handle XBMC clients. Contribute to wirbel-at-vdr-portal/vdr-plugin-vnsiserver-crashes development by creating an account on GitHub.

github.com

git clone https://github.com/wirbel-at-v…in-vnsiserver-crashes.git

identisch zu:

Commits · vdr-projects/vdr-plugin-vnsiserver

VDR plugin to handle XBMC clients. Contribute to vdr-projects/vdr-plugin-vnsiserver development by creating an account on GitHub.

github.com

+ processChannels_GetChannels_StateKey.diff

-----------------------------------------------------------------------------------

-----------------------------------------------------------------------------------

für VDR bitte das hier verwenden:

GitHub - wirbel-at-vdr-portal/vdr-cChannel-Name: temporary git repo for debugging cChannel::Name()

temporary git repo for debugging cChannel::Name(). Contribute to wirbel-at-vdr-portal/vdr-cChannel-Name development by creating an account on GitHub.

github.com

git clone https://github.com/wirbel-at-v…tal/vdr-cChannel-Name.git

identisch zu:

vdr-2.6.6 (ohne Patches)

+ changes in cChannels.c for cChannel::Name() (wirbel + kls)

-----------------------------------------------------------------------------------

Du kannst aus beiden repos auch einfach die letzten commits als Patche downloaden..

Wäre nicht sinnvoll wenn du den Schutz gegen Selbstzuweisen im cString noch aufnimmst? Meiner Meinung nach sollte kls das auf jeden Fall auch aufnehmen.

M-Reimer - danke! Sehe ich ganz genau so, ich hatte das schon gar nicht mehr auf dem Schirm.

Dieser Patch von MarkusE+SHF sollte auf jeden Fall in die nächste VDR Version.

Habe ich in mein temporäres git aufgenommen.

Um so mehr hat RHS mit seinem Kommentar/Nachfrage recht..

Ich denke, die dynamische Generierung von nameSource (und shortNameSource) generell problematisch.

(Sorry, ich hätte das wohl etwas ausführlicher schreiben sollen, war aber gestern etwas in Eile.)

(Btw.: Danke an heifisch fürs testen! )

So wie ich es verstanden habe, sind die cChannel einmalig und quasi statisch (sonst würde das nameSourceMode ja keinen Sinn machen).

Solange ich einen einen Readlock auf der Kanallist habe, muss ich mich darauf verlassen können, dass sich in der Liste nichts ändert.

Bei der Generierung von nameSource ändere ich aber (wahrscheinlich) auch den Pointer darauf. Sobald mehrere Threads gleichzeitig auf die Kanalliste zugreifen besteht da immer die Chance, dass da einer einen ungültigen Pointer bekommt.

Und ich denke, das passiert hier auch gerade.

Die einzige Möglichkeit, wie man das wirklich sauber lösen kann, ist die Generierung von nameSource in einem Writelock auszuführen. Das geht natürlich nicht, solange ein Readlock existiert, man müsste das also in eine eigene Funktion auslagern und bei Bedarf aufrufen.

Das wäre dann bei erstellen den cChannel-Objekts und bei Änderung von Setup.ShowChannelNamesWithSource.

Letzteres passiert aber wohl nur im Menü durch den Nutzer? Wäre also überschaubar.

Insgesamt müsste man wohl etwas mehr umbauen, befürchte ich, es ist aber zumindest einzige Möglichkeit, das wirklich zu lösen, die mir einfällt.

Quote from M-Reimer

Meiner Meinung nach sollte kls das auf jeden Fall auch aufnehmen.

Hab ich schon. Das GIT wird aber erst upgedatet, wenn ich noch weitere Sachen eingebaut habe.

SHF da hast du wohl recht.