Merge pull request xbmc#25590 from ksooo/pvr-fix-clientid-persist

[PVR] Fix major design flaw to use std::hash value as persistent client UID

xbmc/addons/binary-addons/AddonInstanceHandler.cpp

@@ -79,6 +79,11 @@ std::string IAddonInstanceHandler::ID() const
   return m_addon ? m_addon->ID() : "";
 }
 
+AddonInstanceId IAddonInstanceHandler::InstanceID() const
+{
+  return m_instanceId;
+}
+
 std::string IAddonInstanceHandler::Name() const
 {
   return m_addon ? m_addon->Name() : "";

xbmc/addons/binary-addons/AddonInstanceHandler.h

@@ -70,6 +70,7 @@ class IAddonInstanceHandler
   const std::string& UniqueWorkID() { return m_uniqueWorkID; }
 
   std::string ID() const;
+  AddonInstanceId InstanceID() const;
   std::string Name() const;
   std::string Author() const;
   std::string Icon() const;

xbmc/pvr/PVRDatabase.cpp

@@ -9,8 +9,12 @@
 #include "PVRDatabase.h"
 
 #include "ServiceBroker.h"
+#include "addons/AddonManager.h"
+#include "addons/addoninfo/AddonInfo.h"
+#include "addons/addoninfo/AddonType.h"
 #include "dbwrappers/dataset.h"
 #include "pvr/addons/PVRClient.h"
+#include "pvr/addons/PVRClientUID.h"
 #include "pvr/channels/PVRChannel.h"
 #include "pvr/channels/PVRChannelGroupFactory.h"
 #include "pvr/channels/PVRChannelGroupMember.h"
@@ -29,6 +33,7 @@
 #include <memory>
 #include <mutex>
 #include <string>
+#include <tuple>
 #include <utility>
 #include <vector>
 
@@ -191,12 +196,12 @@ void CPVRDatabase::CreateTables()
   );
 
   CLog::LogFC(LOGDEBUG, LOGPVR, "Creating table 'clients'");
-  m_pDS->exec(
-      "CREATE TABLE clients ("
-        "idClient  integer primary key, "
-        "iPriority integer"
-      ")"
-  );
+  m_pDS->exec("CREATE TABLE clients ("
+              "idClient  integer primary key, "
+              "iPriority integer, "
+              "sAddonID TEXT, "
+              "iInstanceID integer"
+              ")");
 
   CLog::LogFC(LOGDEBUG, LOGPVR, "Creating table 'timers'");
   m_pDS->exec(sqlCreateTimersTable);
@@ -376,6 +381,14 @@ void CPVRDatabase::UpdateTables(int iVersion)
     m_pDS->exec("ALTER TABLE channels ADD sDateTimeAdded varchar(20)");
     m_pDS->exec("UPDATE channels SET sDateTimeAdded = ''");
   }
+
+  if (iVersion < 46)
+  {
+    m_pDS->exec("ALTER TABLE clients ADD sAddonID TEXT");
+    m_pDS->exec("ALTER TABLE clients ADD iInstanceID integer");
+
+    FixupClientIDs();
+  }
 }
 
 /********** Client methods **********/
@@ -397,10 +410,10 @@ bool CPVRDatabase::Persist(const CPVRClient& client)
 
   std::unique_lock<CCriticalSection> lock(m_critSection);
 
-  const std::string strQuery = PrepareSQL("REPLACE INTO clients (idClient, iPriority) VALUES (%i, %i);",
-                                          client.GetID(), client.GetPriority());
-
-  return ExecuteQuery(strQuery);
+  const std::string sql{PrepareSQL(
+      "REPLACE INTO clients (idClient, iPriority, sAddonID, iInstanceID) VALUES (%i, %i, '%s', %i)",
+      client.GetID(), client.GetPriority(), client.ID().c_str(), client.InstanceID())};
+  return ExecuteQuery(sql);
 }
 
 bool CPVRDatabase::Delete(const CPVRClient& client)
@@ -436,6 +449,113 @@ int CPVRDatabase::GetPriority(const CPVRClient& client) const
   return atoi(strValue.c_str());
 }
 
+void CPVRDatabase::FixupClientIDs()
+{
+  // Get enabled and disabled PVR client addon infos
+  std::vector<ADDON::AddonInfoPtr> addonInfos;
+  CServiceBroker::GetAddonMgr().GetAddonInfos(addonInfos, false, ADDON::AddonType::PVRDLL);
+
+  std::vector<std::tuple<std::string, ADDON::AddonInstanceId, std::string>> clientInfos;
+  for (const auto& addonInfo : addonInfos)
+  {
+    const std::vector<ADDON::AddonInstanceId> instanceIds{addonInfo->GetKnownInstanceIds()};
+    for (const auto instanceId : instanceIds)
+    {
+      clientInfos.emplace_back(addonInfo->ID(), instanceId, addonInfo->Name());
+    }
+  }
+
+  for (const auto& [addonID, instanceID, addonName] : clientInfos)
+  {
+    // Entry with legacy client id present in clients or channels table?
+    const CPVRClientUID uid{addonID, instanceID};
+    int legacyID{uid.GetLegacyUID()};
+    std::string sql{PrepareSQL("idClient = %i", legacyID)};
+    int id{GetSingleValueInt("clients", "idClient", sql)};
+    if (id == legacyID)
+    {
+      // Add addon id and instance id to existing clients table entry.
+      sql = PrepareSQL("UPDATE clients SET sAddonID = '%s', iInstanceID = %i WHERE idClient = %i",
+                       addonID.c_str(), instanceID, legacyID);
+      ExecuteQuery(sql);
+    }
+    else
+    {
+      sql = PrepareSQL("iClientId = %i", legacyID);
+      id = GetSingleValueInt("channels", "iClientId", sql);
+      if (id == legacyID)
+      {
+        // Create a new entry with the legacy client id in clients table.
+        sql = PrepareSQL("REPLACE INTO clients (idClient, iPriority, sAddonID, iInstanceID) VALUES "
+                         "(%i, %i, '%s', %i)",
+                         legacyID, 0, addonID.c_str(), instanceID);
+        ExecuteQuery(sql);
+      }
+      else
+      {
+        // The legacy id was not found in channels table. This happens if the std::hash
+        // implementation changed (for example after a Kodi update), which according to std::hash
+        // documentation can happen: "Hash functions are only required to produce the same result
+        // for the same input within a single execution of a program"
+
+        // We can only fix some of the ids in this case: We can try to find the legacy id via the
+        // addon's name in the providers table. This is not guaranteed to always work (theoretically
+        // addon name might have changed) and cannot work if providers table contains more than one
+        // entry for the same multi-instance addon.
+
+        sql = PrepareSQL("SELECT iClientId FROM providers WHERE iType = 1 AND sName = '%s'",
+                         addonName.c_str());
+
+        if (ResultQuery(sql))
+        {
+          if (m_pDS->num_rows() != 1)
+          {
+            CLog::Log(LOGERROR, "Unable to fixup client id {} for addon '{}', instance {}!",
+                      legacyID, addonID.c_str(), instanceID);
+          }
+          else
+          {
+            // There is exactly one provider with the addon name in question.
+            // Its client id is the legacy id we're looking for!
+            try
+            {
+              legacyID = m_pDS->fv("iClientId").get_asInt();
+              sql = PrepareSQL(
+                  "REPLACE INTO clients (idClient, iPriority, sAddonID, iInstanceID) VALUES "
+                  "(%i, %i, '%s', %i)",
+                  legacyID, 0, addonID.c_str(), instanceID);
+              ExecuteQuery(sql);
+            }
+            catch (...)
+            {
+              CLog::LogF(LOGERROR, "Couldn't obtain providers for addon '{}'.", addonID);
+            }
+          }
+        }
+      }
+    }
+  }
+}
+
+int CPVRDatabase::GetClientID(const std::string& addonID, unsigned int instanceID)
+{
+  std::unique_lock<CCriticalSection> lock(m_critSection);
+
+  // Client id already present in clients table?
+  std::string sql{PrepareSQL("sAddonID = '%s' AND iInstanceID = %i", addonID.c_str(), instanceID)};
+  const std::string idString{GetSingleValue("clients", "idClient", sql)};
+  if (!idString.empty())
+    return std::atoi(idString.c_str());
+
+  // Create a new entry with a new client id in clients table.
+  sql = PrepareSQL("INSERT INTO clients (iPriority, sAddonID, iInstanceID) VALUES (%i, '%s', %i)",
+                   0, addonID.c_str(), instanceID);
+  if (ExecuteQuery(sql))
+    return static_cast<int>(m_pDS->lastinsertid());
+
+  return -1;
+}
+
 /********** Channel provider methods **********/
 
 bool CPVRDatabase::DeleteProviders()

xbmc/pvr/PVRDatabase.h

@@ -64,7 +64,7 @@ namespace PVR
      * @brief Get the minimal database version that is required to operate correctly.
      * @return The minimal database version.
      */
-    int GetSchemaVersion() const override { return 45; }
+    int GetSchemaVersion() const override { return 46; }
 
     /*!
      * @brief Get the default sqlite database filename.
@@ -102,6 +102,14 @@ namespace PVR
      */
     int GetPriority(const CPVRClient& client) const;
 
+    /*!
+     * @brief Get the numeric client ID for given addon ID and instance ID from the database.
+     * @param addonID The addon ID.
+     * @param instanceID The instance ID.
+     * @return The client ID on success, -1 otherwise.
+     */
+    int GetClientID(const std::string& addonID, unsigned int instanceID);
+
     /*! @name Channel methods */
     //@{
 
@@ -327,6 +335,8 @@ namespace PVR
 
     bool RemoveChannelsFromGroup(const CPVRChannelGroup& group);
 
+    void FixupClientIDs();
+
     mutable CCriticalSection m_critSection;
   };
 }

xbmc/pvr/addons/PVRClientUID.cpp

@@ -8,26 +8,68 @@
 
 #include "PVRClientUID.h"
 
+#include "pvr/PVRDatabase.h"
+#include "utils/log.h"
+
 #include <functional>
+#include <map>
+#include <utility>
 
 using namespace PVR;
 
+namespace
+{
+using ClientUIDParts = std::pair<std::string, ADDON::AddonInstanceId>;
+static std::map<ClientUIDParts, int> s_idMap;
+} // unnamed namespace
+
 int CPVRClientUID::GetUID() const
 {
   if (!m_uidCreated)
   {
-    std::hash<std::string> hasher;
+    const auto it = s_idMap.find({m_addonID, m_instanceID});
+    if (it != s_idMap.cend())
+    {
+      // Cache hit
+      m_uid = (*it).second;
+    }
+    else
+    {
+      // Cache miss. Read from db and cache.
+      CPVRDatabase db;
+      if (!db.Open())
+      {
+        CLog::LogF(LOGERROR, "Unable to open TV database!");
+        return -1;
+      }
 
-    // Note: For database backwards compatibility reasons the hash of the first instance
-    // must be calculated just from the addonId, not from addonId and instanceId.
-    m_uid = static_cast<int>(hasher(
-        (m_instanceID > ADDON::ADDON_FIRST_INSTANCE_ID ? std::to_string(m_instanceID) + "@" : "") +
-        m_addonID));
-    if (m_uid < 0)
-      m_uid = -m_uid;
+      m_uid = db.GetClientID(m_addonID, m_instanceID);
+      if (m_uid == -1)
+      {
+        CLog::LogF(LOGERROR, "Unable to get client id from TV database!");
+        return -1;
+      }
 
+      s_idMap.insert({{m_addonID, m_instanceID}, m_uid});
+    }
     m_uidCreated = true;
   }
 
   return m_uid;
 }
+
+int CPVRClientUID::GetLegacyUID() const
+{
+  // Note: For database backwards compatibility reasons the hash of the first instance
+  // must be calculated just from the addonId, not from addonId and instanceId.
+  std::string prefix;
+  if (m_instanceID > ADDON::ADDON_FIRST_INSTANCE_ID)
+    prefix = std::to_string(m_instanceID) + "@";
+
+  std::hash<std::string> hasher;
+  int uid{static_cast<int>(hasher(prefix + m_addonID))};
+  if (uid < 0)
+    uid = -uid;
+
+  return uid;
+}

xbmc/pvr/addons/PVRClientUID.h

@@ -30,6 +30,12 @@ class CPVRClientUID final
    */
   int GetUID() const;
 
+  /*!
+   * @brief Return the numeric legacy UID (compatibility/migration purposes only).
+   * @return The numeric legacy UID.
+   */
+  int GetLegacyUID() const;
+
 private:
   CPVRClientUID() = delete;
 

xbmc/pvr/addons/PVRClients.cpp

@@ -126,9 +126,17 @@ void CPVRClients::UpdateClients(
             instanceEnabled = client->IsEnabled();
 
           if (instanceEnabled)
+          {
+            CLog::LogF(LOGINFO, "Creating PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToCreate.emplace_back(client);
+          }
           else if (isKnownClient)
+          {
+            CLog::LogF(LOGINFO, "Destroying PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToDestroy.emplace_back(clientId);
+          }
         }
         else if (IsCreatedClient(clientId))
         {
@@ -140,9 +148,17 @@ void CPVRClients::UpdateClients(
           }
 
           if (instanceEnabled)
+          {
+            CLog::LogF(LOGINFO, "Recreating PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToReCreate.emplace_back(clientId, addon->Name());
+          }
           else
+          {
+            CLog::LogF(LOGINFO, "Destroying PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToDestroy.emplace_back(clientId);
+          }
         }
       }
     }