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

[Stefan Hajnoczi <stefanha@gmail.com>]

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 <kwolf@redhat.com>
> ---
>  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.