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

[Kevin Wolf]

Am 05.06.2013 um 14:28 hat Stefan Hajnoczi geschrieben:
> 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 :-).

Ah yes, didn't notice that.

Doesn't really matter though, at the end of the series cmd.c is gone.

> > --- 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.

Do you plan to use this in different context? Because this series is
exactly the opposite of keeping it generic. It moves everything directly
into qemu-io.

Kevin