[RFC,v2,1/4] block: refactor bdrv_next_node for readability

Commit Message

Dou Liyang Dec. 19, 2016, 8:51 a.m. UTC

make the bdrv_next_node() clearly and add some comments.

Signed-off-by: Dou Liyang <[e-mail]>
---
 block.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Stefan Hajnoczi Dec. 19, 2016, 2:19 p.m. UTC | #1

On Mon, Dec 19, 2016 at 04:51:23PM +0800, Dou Liyang wrote:
> make the bdrv_next_node() clearly and add some comments.
> 
> Signed-off-by: Dou Liyang <[e-mail]>
> ---
>  block.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 39ddea3..01c9e51 100644
> --- a/block.c
> +++ b/block.c
> @@ -2931,12 +2931,20 @@ bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
>      return top != NULL;
>  }
>  
> +/*
> + * Return the BlockDriverStates of all the named nodes.

This sentence describes how this function is used in a loop.  The
semantics of one call to this function are different: only a *single*
named node's BlockDriverState is returned.

> + * If @bs is null, return the first one.
> + * Else, return @bs's next sibling, which may be null.
> + *
> + * To iterate over all BlockDriverStates, do
> + * for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(blk)) {
> + *     ...
> + * }
> + */
>  BlockDriverState *bdrv_next_node(BlockDriverState *bs)
>  {
> -    if (!bs) {
> -        return QTAILQ_FIRST(&graph_bdrv_states);
> -    }
> -    return QTAILQ_NEXT(bs, node_list);
> +    return bs ? QTAILQ_NEXT(bs, node_list)
> +            : QTAILQ_FIRST(&graph_bdrv_states);

The conditional operator (?:) is often considered harder to read than an
if statement.  I see no reason to modify the code.

Patch

diff --git a/block.c b/block.c
index 39ddea3..01c9e51 100644
--- a/block.c
+++ b/block.c
@@ -2931,12 +2931,20 @@  bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
     return top != NULL;
 }
 
+/*
+ * Return the BlockDriverStates of all the named nodes.
+ * If @bs is null, return the first one.
+ * Else, return @bs's next sibling, which may be null.
+ *
+ * To iterate over all BlockDriverStates, do
+ * for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(blk)) {
+ *     ...
+ * }
+ */
 BlockDriverState *bdrv_next_node(BlockDriverState *bs)
 {
-    if (!bs) {
-        return QTAILQ_FIRST(&graph_bdrv_states);
-    }
-    return QTAILQ_NEXT(bs, node_list);
+    return bs ? QTAILQ_NEXT(bs, node_list)
+            : QTAILQ_FIRST(&graph_bdrv_states);
 }
 
 const char *bdrv_get_node_name(const BlockDriverState *bs)