Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions common/consumertable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ ConsumerTable::ConsumerTable(DBConnector *db, string tableName) :
}
catch (...)
{
delete m_subscribe;
if(m_subscribe) {
delete m_subscribe;
m_subscribe = NULL;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no needed to "if (m_subscribe)", it's ok to delete NULL.
As for "m_subscribe = NULL;" it's a good change.

}
}
}

Expand All @@ -47,7 +50,9 @@ ConsumerTable::ConsumerTable(DBConnector *db, string tableName) :

ConsumerTable::~ConsumerTable()
{
delete m_subscribe;
if(m_subscribe) {
delete m_subscribe;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not needed. It's ok to delete NULL in C++

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really depends only OS as well. In past I had issues with this. I checked, it seems for Linux its ok.

}

static string pop_front(queue<RedisReply *> &q)
Expand Down
27 changes: 21 additions & 6 deletions common/scheme.h
Original file line number Diff line number Diff line change
@@ -1,17 +1,32 @@
#ifndef __SCHEME__
#define __SCHEME__


#define _in_
#define _out_
#define _inout_

namespace swss {

#define APPL_DB 0
#define ASIC_DB 1

#define APP_PORT_TABLE_NAME "PORT_TABLE"
#define APP_VLAN_TABLE_NAME "VLAN_TABLE"
#define APP_LAG_TABLE_NAME "LAG_TABLE"
#define APP_INTF_TABLE_NAME "INTF_TABLE"
#define APP_NEIGH_TABLE_NAME "NEIGH_TABLE"
#define APP_ROUTE_TABLE_NAME "ROUTE_TABLE"
#define APP_PORT_TABLE_NAME "PORT_TABLE"
#define APP_VLAN_TABLE_NAME "VLAN_TABLE"
#define APP_LAG_TABLE_NAME "LAG_TABLE"
#define APP_INTF_TABLE_NAME "INTF_TABLE"
#define APP_NEIGH_TABLE_NAME "NEIGH_TABLE"
#define APP_ROUTE_TABLE_NAME "ROUTE_TABLE"

#define APP_TC_TO_QUEUE_MAP_TABLE_NAME "TC_TO_QUEUE_MAP_TABLE"
#define APP_SCHEDULER_TABLE_NAME "SCHEDULER_TABLE"
#define APP_DSCP_TO_TC_MAP_TABLE_NAME "DSCP_TO_TC_MAP_TABLE"
#define APP_QUEUE_TABLE_NAME "QUEUE_TABLE"
#define APP_PORT_QOS_MAP_TABLE_NAME "PORT_QOS_MAP_TABLE"
#define APP_WRED_PROFILE_TABLE_NAME "WRED_PROFILE_TABLE"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in different commit.




Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why so many empty lines?


#define IPV4_NAME "IPv4"
#define IPV6_NAME "IPv6"
Expand Down
19 changes: 16 additions & 3 deletions common/select.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#include <algorithm>
#include "common/selectable.h"
#include "common/logger.h"
#include "common/select.h"
#include <stdio.h>
#include <sys/time.h>
Expand All @@ -8,17 +10,28 @@ using namespace std;

namespace swss {

void Select::addSelectable(Selectable *c)
void Select::addSelectable(_in_ Selectable *selectable)
{
m_objects.push_back(c);
if(std::find(m_objects.begin(), m_objects.end(), selectable) != m_objects.end()) {
SWSS_LOG_WARN("Selectable:%p already been added to the list, ignoring\n.");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"\n." is not needed, we can enforce in logs to add \n at the end but currently it goes to syslog so no need,
also yuo forgot to add "selectable" as parameter since you using %p

return;
}
m_objects.push_back(selectable);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be ASSERT() and in DEBUG mode only. There is no point for runtime check like that in production.

}

void Select::addSelectables(_in_ std::vector<Selectable *> &selectables)
{
for(auto it : selectables) {
addSelectable(it);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against these type of changes. I think that the Select class should do one thing, and adding vector of Selectable * sin't one of them.

}

void Select::addFd(int fd)
{
m_fds.push_back(fd);
}

int Select::select(Selectable **c, int *fd, unsigned int timeout)
int Select::select(_out_ Selectable **c, _out_ int *fd, _in_ unsigned int timeout)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stcheng are you ok with the out in weird convention?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my point of view, i don't think the #define in #define out #define inout is necessary in the code. the type itself indicates that.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably assume that Selectable **c should be out parameter, but int *fd wouldn't tell at all.
I don't think it is wierd, but usefull making declarations obviously clear, and self documenting.
Usually OS APIs, and other low level code have such annotations, especially when there is no documentation.
SAI has this as well.
In Windows I believe there are static analysis tools which will treat absence of in/out as error. I understand this is not Windows, but it has benefits outlined above.

{
struct timeval t = {0, (suseconds_t)(timeout)*1000};
struct timeval *pTimeout = NULL;
Expand Down
7 changes: 4 additions & 3 deletions common/select.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@
#include <hiredis/hiredis.h>
#include "selectable.h"


namespace swss {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the extra space?


class Select
{
public:
/* Add object for select */
void addSelectable(Selectable *c);
void addSelectables(_in_ std::vector<Selectable *> &selectables);
void addSelectable(_in_ Selectable *selectable);

/* Add file-descriptor for select */
void addFd(int fd);
Expand All @@ -28,8 +30,7 @@ class Select
ERROR = 2,
TIMEOUT = 3
};
int select(Selectable **c, int *fd,
unsigned int timeout = std::numeric_limits<unsigned int>::max());
int select(_out_ Selectable **c, _out_ int *fd, _in_ unsigned int timeout = std::numeric_limits<unsigned int>::max());

private:
/* Create a new redisContext, SELECT DB and SUBSRIBE */
Expand Down
2 changes: 1 addition & 1 deletion common/table.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ class Table {
void delField(std::string key, std::string field);

virtual ~Table();
std::string getTableName()const {return m_tableName;};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets keep all code in cpp files ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function isn't in used. Why do you need it?


protected:
/* Return the actual key name as a comibation of tableName:key */
std::string getKeyName(std::string key);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a new patch


std::string getKeyQueueTableName();
std::string getValueQueueTableName();
std::string getOpQueueTableName();
Expand Down