Fixes

- Queued RPC calls would cause crashes due to invalid pointers to request/response and/or controller; this has been fixed
    - PbRpcController now takes ownership of request and response messages and
      will delete them when it itself is being deleted
    - This design mandates that request and response messages for each RPC call
      have to be allocated on the heap.
    - The convention for the Closure 'done' call now is to allocate and pass a
      pointer to the controller object to it which will delete it after use;
      this requires that controller itself be also allocated on the heap
      (NOTE: this is just a convention - not mandatory)
    - All existing RPC calls (in portgroup.cpp) have been changed to follow the
      above convention
- Reordering of queued RPC calls has been fixed
- PortManager is now destroyed at exit; because of this fix the per port temporary capture files are auto-removed at exit
- WinPcapPort destructor no longer deletes the monitor threads because the parent class PcapPort already does it
- Capture does not automatically (and incorrectly) stop after one packet if started immediately after a View Capture operation
- User is prompted to stop transmit on a port first if he tries to apply configuration changes on a port in 'transmit' state

client/port.cpp

@@ -18,6 +18,7 @@ Port::Port(quint32 id, quint32 portGroupId)
     d.mutable_port_id()->set_id(id);
     stats.mutable_port_id()->set_id(id);
     mPortGroupId = portGroupId;
+    capFile_ = NULL;
 }
 
 Port::~Port()

client/port.h

@@ -1,9 +1,10 @@
 #ifndef _PORT_H
 #define _PORT_H
 
-#include <QObject>
-#include <QString>
 #include <QList>
+#include <QString>
+#include <QTemporaryFile>
+
 #include "stream.h"
 
 //class StreamModel;
@@ -16,6 +17,7 @@ class Port : public QObject {
 
     OstProto::Port        d;
     OstProto::PortStats   stats;
+    QTemporaryFile *capFile_;
 
     // FIXME(HI): consider removing mPortId as it is duplicated inside 'd'
     quint32        mPortId;
@@ -66,6 +68,12 @@ public:
         { return stats.state().link_state(); }
 
     OstProto::PortStats    getStats() { return stats; }
+    QTemporaryFile* getCaptureFile() 
+    {
+        delete capFile_;
+        capFile_ = new QTemporaryFile();
+        return capFile_; 
+    }
 
     // FIXME(MED): naming inconsistency - PortConfig/Stream; also retVal
     void updatePortConfig(OstProto::Port *port);

client/portgroup.h

@@ -28,13 +28,13 @@ private:
     QString        mUserAlias;            // user defined
 
     PbRpcChannel    *rpcChannel;
-    PbRpcController *rpcController;
-    PbRpcController *rpcControllerStats;
+    PbRpcController *statsController;
     bool            isGetStatsPending_;
 
-    ::OstProto::OstService::Stub *serviceStub;
+    OstProto::OstService::Stub *serviceStub;
 
-    ::OstProto::PortIdList       portIdList;
+    OstProto::PortIdList       *portIdList_;
+    OstProto::PortStatsList    *portStatsList_;
 
 public: // FIXME(HIGH): member access
     QList<Port*>        mPorts;
@@ -62,36 +62,40 @@ public:
     QAbstractSocket::SocketState state() const
         { return rpcChannel->state(); }    
 
-    void processPortIdList(OstProto::PortIdList *portIdList);
-    void processPortConfigList(OstProto::PortConfigList *portConfigList);
+    void processPortIdList(PbRpcController *controller);
+    void processPortConfigList(PbRpcController *controller);
+
+    void processAddStreamAck(PbRpcController *controller);
+    void processDeleteStreamAck(PbRpcController *controller);
+    void processModifyStreamAck(int portIndex, PbRpcController *controller);
 
     void modifyPort(int portId, bool isExclusive);
-    void processModifyPortAck(int portIndex, OstProto::Ack *ack);
-    void processUpdatedPortConfig(OstProto::PortConfigList *portConfigList);
+    void processModifyPortAck(PbRpcController *controller);
+    void processUpdatedPortConfig(PbRpcController *controller);
 
-    void getStreamIdList(int portIndex = 0, 
-        OstProto::StreamIdList *streamIdList = NULL);
-    void getStreamConfigList(int portIndex = 0,
-        OstProto::StreamConfigList *streamConfigList = NULL);
+    void getStreamIdList();
+    void processStreamIdList(int portIndex, PbRpcController *controller);
+    void getStreamConfigList();
+    void processStreamConfigList(int portIndex, PbRpcController *controller);
 
     void processModifyStreamAck(OstProto::Ack *ack);
 
     void startTx(QList<uint> *portList = NULL);
-    void processStartTxAck(OstProto::Ack *ack);
+    void processStartTxAck(PbRpcController *controller);
     void stopTx(QList<uint> *portList = NULL);
-    void processStopTxAck(OstProto::Ack *ack);
+    void processStopTxAck(PbRpcController *controller);
 
     void startCapture(QList<uint> *portList = NULL);
-    void processStartCaptureAck(OstProto::Ack *ack);
+    void processStartCaptureAck(PbRpcController *controller);
     void stopCapture(QList<uint> *portList = NULL);
-    void processStopCaptureAck(OstProto::Ack *ack);
+    void processStopCaptureAck(PbRpcController *controller);
     void viewCapture(QList<uint> *portList = NULL);
-    void processViewCaptureAck(OstProto::CaptureBuffer *buf, QFile *capFile);
+    void processViewCaptureAck(PbRpcController *controller);
 
     void getPortStats();
-    void processPortStatsList(OstProto::PortStatsList *portStatsList);
+    void processPortStatsList();
     void clearPortStats(QList<uint> *portList = NULL);
-    void processClearStatsAck(OstProto::Ack    *ack);
+    void processClearStatsAck(PbRpcController *controller);
 
 signals:
     void portGroupDataChanged(int portGroupId, int portId = 0xFFFF);
@@ -106,16 +110,8 @@ private slots:
     void on_rpcChannel_error(QAbstractSocket::SocketError socketError);
 
 public slots:
-    void when_configApply(int portIndex, uint *cookie = NULL);
-#if 0 // PB
-    void on_rpcChannel_when_dataAvail();
-#endif
+    void when_configApply(int portIndex);
 
-private:
-#if 0 // PB
-    void ProcessCapabilityInfo(const char *msg, qint32 size);
-    void ProcessMsg(const char *msg, quint32 size);
-#endif
 };
 
 #endif

client/portgrouplist.cpp

@@ -22,12 +22,13 @@ PortGroupList::PortGroupList()
 
 PortGroupList::~PortGroupList()
 {
-    while (!mPortGroups.isEmpty())
-        delete mPortGroups.takeFirst();
-
     delete portStatsModelTester_;
     delete portModelTester_;
     delete streamModelTester_;
+
+    while (!mPortGroups.isEmpty())
+        delete mPortGroups.takeFirst();
+
 }
 
 bool PortGroupList::isPortGroup(const QModelIndex& index)

client/portswindow.cpp

@@ -2,6 +2,7 @@
 
 #include <QInputDialog>
 #include <QItemSelectionModel>
+#include <QMessageBox>
 
 #include "streamconfigdialog.h"
 #include "streamlistdelegate.h"
@@ -289,6 +290,13 @@ void PortsWindow::on_pbApply_clicked()
         goto _exit;
     }
 
+    if (plm->port(curPort).getStats().state().is_transmit_on())
+    {
+        QMessageBox::information(0, "Configuration Change",
+                "Please stop transmit on the port before applying any changes");
+        goto _exit;
+    }
+
     curPortGroup = plm->getPortModel()->parent(curPort);
     if (!curPortGroup.isValid())
     {

rpc/pbrpcchannel.cpp

@@ -70,8 +70,9 @@ void PbRpcChannel::CallMethod(
     if (isPending)
     {
         RpcCall call;
-        qDebug("RpcChannel: queueing method %d since %d is pending", 
-            method->index(), pendingMethodId);
+        qDebug("RpcChannel: queueing method %d since %d is pending; "
+                "queued message = <%s>", 
+                method->index(), pendingMethodId, req->DebugString().c_str());
 
         call.method = method;
         call.controller = controller;
@@ -252,18 +253,19 @@ void PbRpcChannel::on_mpSocket_readyRead()
                 
     }
 
+    done->Run();
+
     pendingMethodId = -1;
     controller = NULL;
     response = NULL;
     isPending = false;
     parsing = false;
 
-    done->Run();
-
     if (pendingCallList.size())
     {
         RpcCall call = pendingCallList.takeFirst();
-        qDebug("RpcChannel: executing queued method %d", call.method->index());
+        qDebug("RpcChannel: executing queued method %d <%s>", 
+                call.method->index(), call.request->DebugString().c_str());
         CallMethod(call.method, call.controller, call.request, call.response,
                 call.done);
     }

rpc/pbrpccontroller.h

@@ -5,17 +5,26 @@
 
 class QIODevice;
 
+/*!
+PbRpcController takes ownership of the 'request' and 'response' messages and
+will delete them when it itself is destroyed
+*/
 class PbRpcController : public ::google::protobuf::RpcController
 {
-    bool        failed;
-    QIODevice    *blob;
-    std::string    errStr;
-
 public:
-    PbRpcController() { Reset(); }
+    PbRpcController(::google::protobuf::Message *request, 
+            ::google::protobuf::Message *response) { 
+        request_ = request;
+        response_ = response;
+        Reset(); 
+    }
+    ~PbRpcController() { delete request_; delete response_; }
+
+    ::google::protobuf::Message* request() { return request_; }
+    ::google::protobuf::Message* response() { return response_; }
 
     // Client Side Methods
-    void Reset() { failed=false; blob = NULL; }
+    void Reset() { failed = false; blob = NULL; }
     bool Failed() const { return failed; }
     void StartCancel() { /*! \todo (MED) */}
     std::string ErrorText() const { return errStr; }
@@ -31,6 +40,14 @@ public:
     // srivatsp added
     QIODevice* binaryBlob() { return blob; };
     void setBinaryBlob(QIODevice *binaryBlob) { blob = binaryBlob; };
+
+private:
+    bool failed;
+    QIODevice *blob;
+    std::string errStr;
+    ::google::protobuf::Message *request_;
+    ::google::protobuf::Message *response_;
+
 };
 
 #endif

rpc/rpcserver.cpp

@@ -10,7 +10,6 @@ RpcServer::RpcServer()
 
     isPending = false;
     pendingMethodId = -1; // don't care as long as isPending is false
-    controller_.Reset();
 }
 
 RpcServer::~RpcServer()
@@ -47,22 +46,22 @@ QString RpcServer::errorString()
     return errorString_;
 }
 
-void RpcServer::done(::google::protobuf::Message *request, 
-        ::google::protobuf::Message *response)
+void RpcServer::done(PbRpcController *controller)
 {
+    google::protobuf::Message *response = controller->response();
     QIODevice *blob;
     char msg[MSGBUF_SIZE];
     int len;
 
     //qDebug("In RpcServer::done");
 
-    if (controller_.Failed())
+    if (controller->Failed())
     {
         qDebug("rpc failed");
         goto _exit;
     }
 
-    blob = controller_.binaryBlob();
+    blob = controller->binaryBlob();
     if (blob)
     {
         len = blob->size();
@@ -114,8 +113,7 @@ void RpcServer::done(::google::protobuf::Message *request,
     clientSock->write(msg, PB_HDR_SIZE + len);
 
 _exit:
-    delete request;
-    delete response;
+    delete controller;
     isPending = false;
 }
 
@@ -176,6 +174,7 @@ void RpcServer::when_dataAvail()
     static quint32 len;
     const ::google::protobuf::MethodDescriptor    *methodDesc;
     ::google::protobuf::Message    *req, *resp;
+    PbRpcController *controller;
 
     if (!parsing)
     {
@@ -240,12 +239,12 @@ void RpcServer::when_dataAvail()
     //qDebug("Server(%s): successfully parsed as <%s>", __FUNCTION__, 
         //resp->DebugString().c_str());
 
-    controller_.Reset();
+    controller = new PbRpcController(req, resp);
 
     //qDebug("before service->callmethod()");
 
-    service->CallMethod(methodDesc, &controller_, req, resp,
-        NewCallback(this, &RpcServer::done, req, resp));
+    service->CallMethod(methodDesc, controller, req, resp,
+        google::protobuf::NewCallback(this, &RpcServer::done, controller));
 
     parsing = false;
 

rpc/rpcserver.h

@@ -23,7 +23,6 @@ class RpcServer : public QObject
 
     bool isPending;
     int pendingMethodId;
-    PbRpcController controller_;
     QString errorString_;
 
 public:
@@ -33,8 +32,7 @@ public:
     bool registerService(::google::protobuf::Service *service,
         quint16 tcpPortNum);
     QString errorString();
-    void done(::google::protobuf::Message *req, 
-            ::google::protobuf::Message *resp);
+    void done(PbRpcController *controller);
 
 private slots:
      void when_newConnection();

server/myservice.cpp

@@ -25,6 +25,9 @@ MyService::MyService()
 
 MyService::~MyService()
 {
+    //! \todo Use a singleton destroyer instead 
+    // http://www.research.ibm.com/designpatterns/pubs/ph-jun96.txt
+    delete PortManager::instance();
 }
 
 void MyService::getPortIdList(::google::protobuf::RpcController* /*controller*/,

server/pcapport.cpp

@@ -57,6 +57,7 @@ void PcapPort::init()
 
 PcapPort::~PcapPort()
 {
+    qDebug("In %s", __FUNCTION__);
     delete capturer_;
     delete transmitter_;
     delete monitorTx_;
@@ -296,7 +297,8 @@ _restart:
 
         if (ret < 0)
         {
-            qDebug("error in sendQueueTransmit()");
+            qDebug("error %d in sendQueueTransmit()", ret);
+            stop_ = false;
             return;
         }
     }
@@ -312,7 +314,8 @@ _restart:
 
 void PcapPort::PortTransmitter::stop()
 {
-    stop_ = true;
+    if (isRunning())
+        stop_ = true;
 }
 
 int PcapPort::PortTransmitter::sendQueueTransmit(pcap_t *p,
@@ -332,7 +335,6 @@ int PcapPort::PortTransmitter::sendQueueTransmit(pcap_t *p,
 
         if (stop_)
         {
-            stop_ = false;
             return -2;
         }
 
@@ -453,7 +455,6 @@ void PcapPort::PortCapturer::run()
         if (stop_) 
         {
             qDebug("user requested capture stop\n");
-            stop_ = false;
             break;
         }
     }
@@ -461,11 +462,13 @@ void PcapPort::PortCapturer::run()
     pcap_close(handle_);
     dumpHandle_ = NULL;
     handle_ = NULL;
+    stop_ = false;
 }
 
 void PcapPort::PortCapturer::stop()
 {
-    stop_ = true;
+    if (isRunning())
+        stop_ = true;
 }
 
 QFile* PcapPort::PortCapturer::captureFile()

server/pcapport.h

@@ -95,7 +95,7 @@ protected:
         AbstractPort::PortStats *stats_;
         bool usingInternalHandle_;
         pcap_t *handle_;
-        bool stop_;
+        volatile bool stop_;
     };
 
     class PortCapturer: public QThread
@@ -109,7 +109,7 @@ protected:
 
     private:
         QString         device_;
-        bool            stop_;
+        volatile bool   stop_;
         QTemporaryFile  capFile_;
         pcap_t          *handle_;
         pcap_dumper_t   *dumpHandle_;

server/winpcapport.cpp

@@ -28,8 +28,6 @@ WinPcapPort::WinPcapPort(int id, const char *device)
 
 WinPcapPort::~WinPcapPort()
 {
-    delete monitorRx_;
-    delete monitorTx_;
 }
 
 OstProto::LinkState WinPcapPort::linkState()