Add scheduler groups support to virtual switch #132
Conversation
| @@ -84,6 +84,12 @@ sai_status_t transfer_list( | |||
| { | |||
| transfer_primitive(src_element.count, dst_element.count); | |||
|
|
|||
| if (src_element.list == NULL && src_element.count > 0) | |||
There was a problem hiding this comment.
if (src_element.list == NULL && src_element.count > 0) [](start = 7, length = 55)
replace src with dst, and move if-block before previous statement? #Closed
There was a problem hiding this comment.
no, src must be check since from it it will be copy
There was a problem hiding this comment.
Sorry, I thought the wrong direction. So move if-block before previous statement? So that there is no partially assigned dst_element when throw? #Closed
There was a problem hiding this comment.
I don't quite get that, when you will get currupted dst_element ?
There was a problem hiding this comment.
(update the original comment to clarify) #Closed
There was a problem hiding this comment.
Ah in this case, sure i can move that, anyway this partial override doesen't matter since api will fail
| @@ -20,6 +20,12 @@ if SAICAVM | |||
| SAIFLAGS = -DCAVMSAI | |||
| endif | |||
|
|
|||
| if SAIVS | |||
| SAILIB=-L$(top_srcdir)/vslib/src/.libs -lsaivs | |||
There was a problem hiding this comment.
.libs [](start = 33, length = 5)
.libs is not necessary. This is an example: https://github.com/Azure/sonic-swss-common/blob/master/tests/Makefile.am
There was a problem hiding this comment.
its needed since i want to prefer local build lib agains the one installed in the system which is causing a lot of confusion and require to build twice
There was a problem hiding this comment.
Actually that is actual behavior. libtool does the magic.
There was a problem hiding this comment.
Not sure about this, i will need to investigate on that, for now i leave this as is since its working
|
|
||
| // sg 0 (2 groups) | ||
| { | ||
| sai_object_id_t sg_0 = sgs.at(0); |
There was a problem hiding this comment.
sgs [](start = 31, length = 3)
Do you have assumption on sgs.size()? Add assertion or runtime check?
There was a problem hiding this comment.
at() is runtime check
There was a problem hiding this comment.
I see. It will throw. Is it better to return a sai_status_t?
There was a problem hiding this comment.
Depends how to look at this, since this is one off, if written correctly it will never fail, and if some actions will lead to throw it will need to be corrected right away since this logic is executed in vs tests when doing make check, then is auto tested
There was a problem hiding this comment.
Actually all those CHECK_STATUS could throw anyway
| { | ||
| sai_object_id_t childs[2]; | ||
|
|
||
| childs[0] = queues[queue_index]; // first half are in queues |
There was a problem hiding this comment.
queue_index [](start = 31, length = 11)
i? #Closed
There was a problem hiding this comment.
no, queue_index, analyze code
| @@ -44,13 +46,175 @@ service_method_table_t test_services = { | |||
| profile_get_next_value | |||
| }; | |||
|
|
|||
| #define SUCCESS(x) \ | |||
| if (x != SAI_STATUS_SUCCESS) \ | |||
There was a problem hiding this comment.
x [](start = 8, length = 1)
(x) #Closed
|
|
||
| ASSERT_TRUE(attr.value.objlist.count == expected_ports); | ||
| } | ||
|
|
||
| int main() |
There was a problem hiding this comment.
main [](start = 4, length = 4)
Just wondering why not use gtest?
There was a problem hiding this comment.
you are wellcome to move all tests to gtest
|
this commit is failing for the build check. seems it wants to link with opennsl on the mellanox platform. Not sure why. |
| if SAIVS | ||
| SAILIB=-L$(top_srcdir)/vslib/src/.libs -lsaivs | ||
| else | ||
| SAILIB=-lsai -lopennsl -ldl |
There was a problem hiding this comment.
-lopennsl [](start = 13, length = 9)
this should be brcm specific.
We observe that the change made by installer sometimes fails to be written onto hard disk. This PR will fix this issue.
No description provided.