Device Emulation (contd.): Fixed DeviceGroup related RPCs to be invoked only if required by client when "Apply" is clicked

client/devicegroupdialog.cpp

@@ -219,7 +219,7 @@ void DeviceGroupDialog::updateIp6Gateway()
 
 void DeviceGroupDialog::loadDeviceGroup()
 {
-    OstProto::DeviceGroup *devGrp = port_->deviceGroupByIndex(index_);
+    const OstProto::DeviceGroup *devGrp = port_->deviceGroupByIndex(index_);
     int tagCount = 0;
 
     Q_ASSERT(devGrp);
@@ -285,7 +285,7 @@ void DeviceGroupDialog::loadDeviceGroup()
 
 void DeviceGroupDialog::storeDeviceGroup()
 {
-    OstProto::DeviceGroup *devGrp = port_->deviceGroupByIndex(index_);
+    OstProto::DeviceGroup *devGrp = port_->mutableDeviceGroupByIndex(index_);
     int tagCount = vlanTagCount->value();
 
     Q_ASSERT(devGrp);

client/devicegroupmodel.cpp

@@ -97,7 +97,7 @@ QVariant DeviceGroupModel::data(const QModelIndex &index, int role) const
     Q_ASSERT(dgIdx < port_->numDeviceGroups());
     Q_ASSERT(field < kFieldCount);
 
-    OstProto::DeviceGroup *devGrp = port_->deviceGroupByIndex(dgIdx);
+    const OstProto::DeviceGroup *devGrp = port_->deviceGroupByIndex(dgIdx);
 
     Q_ASSERT(devGrp);
 
@@ -157,8 +157,8 @@ QVariant DeviceGroupModel::data(const QModelIndex &index, int role) const
                 case Qt::DisplayRole:
                     if (devGrp->HasExtension(OstEmul::ip4))
                         return QHostAddress(
-                                    devGrp->MutableExtension(OstEmul::ip4)
+                                    devGrp->GetExtension(OstEmul::ip4)
-                                                    ->address()).toString();
+                                                    .address()).toString();
                     else
                         return QString("--");
                 default:
@@ -170,8 +170,8 @@ QVariant DeviceGroupModel::data(const QModelIndex &index, int role) const
             switch (role) {
                 case Qt::DisplayRole:
                     if (devGrp->HasExtension(OstEmul::ip6)) {
-                        OstEmul::Ip6Address ip = devGrp->MutableExtension(
+                        OstEmul::Ip6Address ip = devGrp->GetExtension(
-                                                    OstEmul::ip6)->address();
+                                                    OstEmul::ip6).address();
                         return QHostAddress(
                                     UInt128(ip.hi(), ip.lo()).toArray())
                                         .toString();

client/port.cpp

@@ -443,12 +443,10 @@ void Port::getNewDeviceGroupsSinceLastSync(
 void Port::getModifiedDeviceGroupsSinceLastSync(
     OstProto::DeviceGroupConfigList &deviceGroupConfigList)
 {
-    // FIXME: we currently don't have any mechanism to check
-    // if a DeviceGroup was modified since last sync, so we
-    // include all DeviceGroups
     deviceGroupConfigList.clear_device_group();
-    foreach(OstProto::DeviceGroup *dg, deviceGroups_)
+    foreach(quint32 id, modifiedDeviceGroupList_)
-        deviceGroupConfigList.add_device_group()->CopyFrom(*dg);
+        deviceGroupConfigList.add_device_group()
+                                ->CopyFrom(*deviceGroupById(id));
 }
 
 void Port::when_syncComplete()
@@ -464,6 +462,7 @@ void Port::when_syncComplete()
         lastSyncDeviceGroupList_.append(
                 deviceGroups_.at(i)->device_group_id().id());
     }
+    modifiedDeviceGroupList_.clear();
 }
 
 void Port::updateStats(OstProto::PortStats *portStats)
@@ -671,12 +670,12 @@ uint Port::newDeviceGroupId()
     return allocDeviceGroupId_++;
 }
 
-int Port::numDeviceGroups()
+int Port::numDeviceGroups() const
 {
     return deviceGroups_.size();
 }
 
-OstProto::DeviceGroup* Port::deviceGroupByIndex(int index)
+const OstProto::DeviceGroup* Port::deviceGroupByIndex(int index) const
 {
     if ((index < 0) || (index >= numDeviceGroups())) {
         qWarning("%s: index %d out of range (0 - %d)", __FUNCTION__,
@@ -687,6 +686,22 @@ OstProto::DeviceGroup* Port::deviceGroupByIndex(int index)
     return deviceGroups_.at(index);
 }
 
+OstProto::DeviceGroup* Port::mutableDeviceGroupByIndex(int index)
+{
+    if ((index < 0) || (index >= numDeviceGroups())) {
+        qWarning("%s: index %d out of range (0 - %d)", __FUNCTION__,
+                index, numDeviceGroups() - 1);
+        return NULL;
+    }
+
+    OstProto::DeviceGroup *devGrp = deviceGroups_.at(index);
+
+    // Caller can modify DeviceGroup - assume she will
+    modifiedDeviceGroupList_.insert(devGrp->device_group_id().id());
+
+    return devGrp;
+}
+
 OstProto::DeviceGroup* Port::deviceGroupById(uint deviceGroupId)
 {
     for (int i = 0; i < deviceGroups_.size(); i++) {
@@ -715,6 +730,7 @@ bool Port::newDeviceGroupAt(int index, const OstProto::DeviceGroup *deviceGroup)
 
     devGrp->mutable_device_group_id()->set_id(newDeviceGroupId());
     deviceGroups_.insert(index, devGrp);
+    modifiedDeviceGroupList_.insert(devGrp->device_group_id().id());
 
     return true;
 }
@@ -724,7 +740,9 @@ bool Port::deleteDeviceGroupAt(int index)
     if (index < 0 || index >= deviceGroups_.size())
         return false;
 
-    delete deviceGroups_.takeAt(index);
+    OstProto::DeviceGroup *devGrp = deviceGroups_.takeAt(index);
+    modifiedDeviceGroupList_.remove(devGrp->device_group_id().id());
+    delete devGrp;
 
     return true;
 }

client/port.h

@@ -23,6 +23,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>
 #include <QDir>
 #include <QHash>
 #include <QList>
+#include <QSet>
 #include <QString>
 #include <QTemporaryFile>
 
@@ -58,6 +59,7 @@ class Port : public QObject {
     QList<Stream*>    mStreams;        // sorted by stream's ordinal value
 
     QList<quint32> lastSyncDeviceGroupList_;
+    QSet<quint32>  modifiedDeviceGroupList_;
     QList<OstProto::DeviceGroup*> deviceGroups_;
     QList<OstEmul::Device*> devices_;
     QHash<quint32, OstEmul::DeviceNeighborList*> deviceNeighbors_;
@@ -168,8 +170,9 @@ public:
     // ------------ Device Group ----------- //
 
     uint newDeviceGroupId();
-    int numDeviceGroups();
+    int numDeviceGroups() const;
-    OstProto::DeviceGroup* deviceGroupByIndex(int index);
+    const OstProto::DeviceGroup* deviceGroupByIndex(int index) const;
+    OstProto::DeviceGroup* mutableDeviceGroupByIndex(int index);
     OstProto::DeviceGroup* deviceGroupById(uint deviceGroupId);
 
     //! Used by StreamModel

client/portgroup.cpp

@@ -394,6 +394,8 @@ void PortGroup::when_configApply(int portIndex)
     // FIXME: as currently written this code will make unnecessary RPCs
     // even if the request contains no data; the fix will need to take
     // care to identify when sync is complete
+    // NOTE: DeviceGroup RPCs are no longer called unnecessarily;
+    // Stream RPCs need to be fixed similarly
     // Also, drone currently updates its packet list at the end of
     // modifyStream() implicitly assuming that will be the last API
     // called - this will also need to be fixed
@@ -406,38 +408,46 @@ void PortGroup::when_configApply(int portIndex)
 
     qDebug("applying 'deleted deviceGroups' ...");
     deviceGroupIdList = new OstProto::DeviceGroupIdList;
-    ack = new OstProto::Ack;
-    controller = new PbRpcController(deviceGroupIdList, ack);
-
     deviceGroupIdList->mutable_port_id()->set_id(mPorts[portIndex]->id());
     mPorts[portIndex]->getDeletedDeviceGroupsSinceLastSync(*deviceGroupIdList);
-
+    if (deviceGroupIdList->device_group_id_size()) {
+        ack = new OstProto::Ack;
+        controller = new PbRpcController(deviceGroupIdList, ack);
         serviceStub->deleteDeviceGroup(controller, deviceGroupIdList, ack,
-        NewCallback(this, &PortGroup::processDeleteDeviceGroupAck, controller));
+            NewCallback(this, &PortGroup::processDeleteDeviceGroupAck,
+                        controller));
+    }
+    else
+        delete deviceGroupIdList;
 
     qDebug("applying 'new deviceGroups' ...");
     deviceGroupIdList = new OstProto::DeviceGroupIdList;
-    ack = new OstProto::Ack;
-    controller = new PbRpcController(deviceGroupIdList, ack);
-
     deviceGroupIdList->mutable_port_id()->set_id(mPorts[portIndex]->id());
     mPorts[portIndex]->getNewDeviceGroupsSinceLastSync(*deviceGroupIdList);
-
+    if (deviceGroupIdList->device_group_id_size()) {
+        ack = new OstProto::Ack;
+        controller = new PbRpcController(deviceGroupIdList, ack);
         serviceStub->addDeviceGroup(controller, deviceGroupIdList, ack,
-        NewCallback(this, &PortGroup::processAddDeviceGroupAck, controller));
+            NewCallback(this, &PortGroup::processAddDeviceGroupAck,
+                        controller));
+    }
+    else
+        delete deviceGroupIdList;
 
     qDebug("applying 'modified deviceGroups' ...");
     deviceGroupConfigList = new OstProto::DeviceGroupConfigList;
-    ack = new OstProto::Ack;
-    controller = new PbRpcController(deviceGroupConfigList, ack);
-
     deviceGroupConfigList->mutable_port_id()->set_id(mPorts[portIndex]->id());
     mPorts[portIndex]->getModifiedDeviceGroupsSinceLastSync(
             *deviceGroupConfigList);
-
+    if (deviceGroupConfigList->device_group_size()) {
+        ack = new OstProto::Ack;
+        controller = new PbRpcController(deviceGroupConfigList, ack);
         serviceStub->modifyDeviceGroup(controller, deviceGroupConfigList, ack,
                 NewCallback(this, &PortGroup::processModifyDeviceGroupAck,
                     portIndex, controller));
+    }
+    else
+        delete deviceGroupConfigList;
 
     //
     // Update/Sync Streams