Re: Review Request 34957: Fixed a bug in qdisc search function.

2015-06-02 Thread Jie Yu 


> On June 2, 2015, 11:09 p.m., Paul Brett wrote:
> > src/linux/routing/queueing/internal.hpp, line 143
> > 
> >
> > Not your problem, but we really should either explain why we have to 
> > increment the reference counter here but nowhere else or just fix Netlink 
> > to hide this (it does hide most of the cleanup logic)
> 
> Jie Yu wrote:
> Yeah, we should definitely add some comments. Can you sugguest how to fix 
> Netlink to hide this?
> 
> Paul Brett wrote:
> Chi and I were talking about adding a copy constructor to Netlink that 
> would increment the reference counter using helper functions like the 
> destructors, but that needs a separate ticket and some more thought.

OK, I added a NOTE to explain why nl_object_get(o) is needed here.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34957/#review86303
---


On June 2, 2015, 10:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34957/
> ---
> 
> (Updated June 2, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Paul Brett, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug in qdisc search function.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/queueing/internal.hpp 
> d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 
> 
> Diff: https://reviews.apache.org/r/34957/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34957: Fixed a bug in qdisc search function.

2015-06-02 Thread Paul Brett 


> On June 2, 2015, 11:09 p.m., Paul Brett wrote:
> > src/linux/routing/queueing/internal.hpp, line 143
> > 
> >
> > Not your problem, but we really should either explain why we have to 
> > increment the reference counter here but nowhere else or just fix Netlink 
> > to hide this (it does hide most of the cleanup logic)
> 
> Jie Yu wrote:
> Yeah, we should definitely add some comments. Can you sugguest how to fix 
> Netlink to hide this?

Chi and I were talking about adding a copy constructor to Netlink that would 
increment the reference counter using helper functions like the destructors, 
but that needs a separate ticket and some more thought.


- Paul


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34957/#review86303
---


On June 2, 2015, 10:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34957/
> ---
> 
> (Updated June 2, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Paul Brett, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug in qdisc search function.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/queueing/internal.hpp 
> d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 
> 
> Diff: https://reviews.apache.org/r/34957/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34957: Fixed a bug in qdisc search function.

2015-06-02 Thread Jie Yu 


> On June 2, 2015, 11:09 p.m., Paul Brett wrote:
> > src/linux/routing/queueing/internal.hpp, line 143
> > 
> >
> > Not your problem, but we really should either explain why we have to 
> > increment the reference counter here but nowhere else or just fix Netlink 
> > to hide this (it does hide most of the cleanup logic)

Yeah, we should definitely add some comments. Can you sugguest how to fix 
Netlink to hide this?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34957/#review86303
---


On June 2, 2015, 10:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34957/
> ---
> 
> (Updated June 2, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Paul Brett, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug in qdisc search function.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/queueing/internal.hpp 
> d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 
> 
> Diff: https://reviews.apache.org/r/34957/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34957: Fixed a bug in qdisc search function.

2015-06-02 Thread Paul Brett 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34957/#review86305
---

Ship it!


Ship It!

- Paul Brett


On June 2, 2015, 10:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34957/
> ---
> 
> (Updated June 2, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Paul Brett, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug in qdisc search function.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/queueing/internal.hpp 
> d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 
> 
> Diff: https://reviews.apache.org/r/34957/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34957: Fixed a bug in qdisc search function.

2015-06-02 Thread Paul Brett 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34957/#review86303
---



src/linux/routing/queueing/internal.hpp


Not your problem, but we really should either explain why we have to 
increment the reference counter here but nowhere else or just fix Netlink to 
hide this (it does hide most of the cleanup logic)


- Paul Brett


On June 2, 2015, 10:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34957/
> ---
> 
> (Updated June 2, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Paul Brett, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug in qdisc search function.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/queueing/internal.hpp 
> d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 
> 
> Diff: https://reviews.apache.org/r/34957/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34957: Fixed a bug in qdisc search function.

2015-06-02 Thread Cong Wang 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34957/#review86301
---

Ship it!


This bug was found by Paul, just FYI.

- Cong Wang


On June 2, 2015, 10:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34957/
> ---
> 
> (Updated June 2, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Paul Brett, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug in qdisc search function.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/queueing/internal.hpp 
> d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 
> 
> Diff: https://reviews.apache.org/r/34957/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>