Re: [Qemu-devel] [PATCH 05/16] qemu-io: Don't use global bs in command implementations

[Stefan Hajnoczi]

On Tue, May 28, 2013 at 05:27:25PM +0200, Kevin Wolf wrote:
> Pass in the BlockDriverState to the command handlers instead of using
> the global variable. This is an important step to make the commands
> usable outside of qemu-io.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  cmd.c     |   6 ++-
>  cmd.h     |   8 ++-
>  qemu-io.c | 165 
> ++++++++++++++++++++++++++++++++++----------------------------
>  3 files changed, 100 insertions(+), 79 deletions(-)
> 
> diff --git a/cmd.c b/cmd.c
> index 214c6f7..d501aab 100644
> --- a/cmd.c
> +++ b/cmd.c
> @@ -57,7 +57,7 @@ check_command(
>       const cmdinfo_t *ci)
>  {
>       if (check_func)
> -             return check_func(ci);
> +             return check_func(qemuio_bs, ci);
>       return 1;
>  }
>  
> @@ -103,7 +103,7 @@ command(
>               return 0;
>       }
>       optind = 0;
> -     return ct->cfunc(argc, argv);
> +     return ct->cfunc(qemuio_bs, argc, argv);
>  }
>  
>  const cmdinfo_t *
> @@ -452,6 +452,7 @@ static cmdinfo_t quit_cmd;
>  /* ARGSUSED */
>  static int
>  quit_f(
> +    BlockDriverState *bs,
>       int     argc,

tabs vs spaces.  I try to keep the existing style unless I decide to
reformat the entire section of code.

Not trying to start a flamewar but this file appears to use tabs and IMO
you should stick to that instead of mixing spaces :-).

>       char    **argv)
>  {
> @@ -490,6 +491,7 @@ help_all(void)
>  
>  static int
>  help_f(
> +    BlockDriverState *bs,
>       int             argc,
>       char            **argv)
>  {
> diff --git a/cmd.h b/cmd.h
> index 4dcfe88..ccf6336 100644
> --- a/cmd.h
> +++ b/cmd.h
> @@ -17,9 +17,13 @@
>  #ifndef __COMMAND_H__
>  #define __COMMAND_H__
>  
> +#include "qemu-common.h"
> +
>  #define CMD_FLAG_GLOBAL      ((int)0x80000000)       /* don't iterate "args" 
> */
>  
> -typedef int (*cfunc_t)(int argc, char **argv);
> +extern BlockDriverState *qemuio_bs;
> +
> +typedef int (*cfunc_t)(BlockDriverState *bs, int argc, char **argv);
>  typedef void (*helpfunc_t)(void);
>  
>  typedef struct cmdinfo {
> @@ -41,7 +45,7 @@ extern int          ncmds;
>  void help_init(void);
>  void quit_init(void);
>  
> -typedef int (*checkfunc_t)(const cmdinfo_t *ci);
> +typedef int (*checkfunc_t)(BlockDriverState *bs, const cmdinfo_t *ci);
>  
>  void add_command(const cmdinfo_t *ci);
>  void add_user_command(char *optarg);

cmd.h does not know about the block layer.  I would use void *opaque
instead of BlockDriverState *bs.  That way the file stays generic and
can be used in other command-line tools.