| Message ID | ce9bc88496a04e6495320b041a0de4e0155b76a9.1482205355.git.geliangtang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 20, 2016 at 10:02:19PM +0800, Geliang Tang wrote: > To make the code clearer, use rb_entry() instead of container_of() to > deal with rbtree. That is OK but I think 'container_of' is more clear. Roger, thoughts? > > Signed-off-by: Geliang Tang <geliangtang@gmail.com> > --- > drivers/block/xen-blkback/blkback.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c > index 726c32e..7e59fae 100644 > --- a/drivers/block/xen-blkback/blkback.c > +++ b/drivers/block/xen-blkback/blkback.c > @@ -191,10 +191,10 @@ static void make_response(struct xen_blkif_ring *ring, u64 id, > unsigned short op, int st); > > #define foreach_grant_safe(pos, n, rbtree, node) \ > - for ((pos) = container_of(rb_first((rbtree)), typeof(*(pos)), node), \ > + for ((pos) = rb_entry(rb_first((rbtree)), typeof(*(pos)), node), \ > (n) = (&(pos)->node != NULL) ? rb_next(&(pos)->node) : NULL; \ > &(pos)->node != NULL; \ > - (pos) = container_of(n, typeof(*(pos)), node), \ > + (pos) = rb_entry(n, typeof(*(pos)), node), \ > (n) = (&(pos)->node != NULL) ? rb_next(&(pos)->node) : NULL) > > > @@ -223,7 +223,7 @@ static int add_persistent_gnt(struct xen_blkif_ring *ring, > /* Figure out where to put new node */ > new = &ring->persistent_gnts.rb_node; > while (*new) { > - this = container_of(*new, struct persistent_gnt, node); > + this = rb_entry(*new, struct persistent_gnt, node); > > parent = *new; > if (persistent_gnt->gnt < this->gnt) > @@ -254,7 +254,7 @@ static struct persistent_gnt *get_persistent_gnt(struct xen_blkif_ring *ring, > > node = ring->persistent_gnts.rb_node; > while (node) { > - data = container_of(node, struct persistent_gnt, node); > + data = rb_entry(node, struct persistent_gnt, node); > > if (gref < data->gnt) > node = node->rb_left; > -- > 2.9.3 >
On Tue, Dec 20, 2016 at 11:47:03AM -0500, Konrad Rzeszutek Wilk wrote: > On Tue, Dec 20, 2016 at 10:02:19PM +0800, Geliang Tang wrote: > > To make the code clearer, use rb_entry() instead of container_of() to > > deal with rbtree. > > That is OK but I think 'container_of' is more clear. > > Roger, thoughts? I think so, container_of is a global macro that's widely used and everyone knows, rb_entry OTOH it's not and it's use doesn't really simply the code at all. I'm not really opposed, but it seems kind of a pointless change (not that it's wrong). Roger.
On Tue, Dec 20, 2016 at 05:44:06PM +0000, Roger Pau Monné wrote: > On Tue, Dec 20, 2016 at 11:47:03AM -0500, Konrad Rzeszutek Wilk wrote: > > On Tue, Dec 20, 2016 at 10:02:19PM +0800, Geliang Tang wrote: > > > To make the code clearer, use rb_entry() instead of container_of() to > > > deal with rbtree. > > > > That is OK but I think 'container_of' is more clear. > > > > Roger, thoughts? > > I think so, container_of is a global macro that's widely used and everyone > knows, rb_entry OTOH it's not and it's use doesn't really simply the code at > all. I'm not really opposed, but it seems kind of a pointless change (not that > it's wrong). <nods> I concur. Geliang Tang, Thank you for the patch but there is no need for it. Thanks again! > > Roger.
On Tue, 2016-12-20 at 12:51 -0500, Konrad Rzeszutek Wilk wrote: > On Tue, Dec 20, 2016 at 05:44:06PM +0000, Roger Pau Monné wrote: > > On Tue, Dec 20, 2016 at 11:47:03AM -0500, Konrad Rzeszutek Wilk wrote: > > > On Tue, Dec 20, 2016 at 10:02:19PM +0800, Geliang Tang wrote: > > > > To make the code clearer, use rb_entry() instead of container_of() to > > > > deal with rbtree. > > > > > > That is OK but I think 'container_of' is more clear. > > > > > > Roger, thoughts? > > > > I think so, container_of is a global macro that's widely used and everyone > > knows, rb_entry OTOH it's not and it's use doesn't really simply the code at > > all. I'm not really opposed, but it seems kind of a pointless change (not that > > it's wrong). > > <nods> I concur. > > Geliang Tang, > > Thank you for the patch but there is no need for it. The same could be said of list_entry() #define hlist_entry(ptr, type, member) container_of(ptr,type,member) #define list_entry(ptr, type, member) container_of(ptr, type, member) # git grep -n list_entry | wc -l 3636 rb_entry() will probably make its way everywhere.
On Tue, Dec 20, 2016 at 01:53:21PM -0800, Eric Dumazet wrote: > On Tue, 2016-12-20 at 12:51 -0500, Konrad Rzeszutek Wilk wrote: > > On Tue, Dec 20, 2016 at 05:44:06PM +0000, Roger Pau Monné wrote: > > > On Tue, Dec 20, 2016 at 11:47:03AM -0500, Konrad Rzeszutek Wilk wrote: > > > > On Tue, Dec 20, 2016 at 10:02:19PM +0800, Geliang Tang wrote: > > > > > To make the code clearer, use rb_entry() instead of container_of() to > > > > > deal with rbtree. > > > > > > > > That is OK but I think 'container_of' is more clear. > > > > > > > > Roger, thoughts? > > > > > > I think so, container_of is a global macro that's widely used and everyone > > > knows, rb_entry OTOH it's not and it's use doesn't really simply the code at > > > all. I'm not really opposed, but it seems kind of a pointless change (not that > > > it's wrong). > > > > <nods> I concur. > > > > Geliang Tang, > > > > Thank you for the patch but there is no need for it. > > The same could be said of list_entry() > Sure. And I am used to that as well :-) > #define hlist_entry(ptr, type, member) container_of(ptr,type,member) > > #define list_entry(ptr, type, member) container_of(ptr, type, member) > > # git grep -n list_entry | wc -l > 3636 > > rb_entry() will probably make its way everywhere. That is good to know. But for right now this patch is not necessary. Thank you!
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c index 726c32e..7e59fae 100644 --- a/drivers/block/xen-blkback/blkback.c +++ b/drivers/block/xen-blkback/blkback.c @@ -191,10 +191,10 @@ static void make_response(struct xen_blkif_ring *ring, u64 id, unsigned short op, int st); #define foreach_grant_safe(pos, n, rbtree, node) \ - for ((pos) = container_of(rb_first((rbtree)), typeof(*(pos)), node), \ + for ((pos) = rb_entry(rb_first((rbtree)), typeof(*(pos)), node), \ (n) = (&(pos)->node != NULL) ? rb_next(&(pos)->node) : NULL; \ &(pos)->node != NULL; \ - (pos) = container_of(n, typeof(*(pos)), node), \ + (pos) = rb_entry(n, typeof(*(pos)), node), \ (n) = (&(pos)->node != NULL) ? rb_next(&(pos)->node) : NULL) @@ -223,7 +223,7 @@ static int add_persistent_gnt(struct xen_blkif_ring *ring, /* Figure out where to put new node */ new = &ring->persistent_gnts.rb_node; while (*new) { - this = container_of(*new, struct persistent_gnt, node); + this = rb_entry(*new, struct persistent_gnt, node); parent = *new; if (persistent_gnt->gnt < this->gnt) @@ -254,7 +254,7 @@ static struct persistent_gnt *get_persistent_gnt(struct xen_blkif_ring *ring, node = ring->persistent_gnts.rb_node; while (node) { - data = container_of(node, struct persistent_gnt, node); + data = rb_entry(node, struct persistent_gnt, node); if (gref < data->gnt) node = node->rb_left;
To make the code clearer, use rb_entry() instead of container_of() to deal with rbtree. Signed-off-by: Geliang Tang <geliangtang@gmail.com> --- drivers/block/xen-blkback/blkback.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)