Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

2013-07-10 Thread Wenchao Xia 

于 2013-7-9 22:14, Luiz Capitulino 写道:

On Tue, 09 Jul 2013 22:03:42 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:




You could pass cur_mon to readline_completion() in readline_handle_byte()
to avoid all this, but it would be preferable to clarify the matter.

This is also another benefit of having readline_completion() in a
different patch, you can (and should!) clarify this point in the commit
log along with testing results.


OK, I'll test multiple monitors case.




Do you have an example about how invoke qemu with multiple monitor?
Do you have recommendation about the way to connect to qemu's moniotr?
I tried pty with minicom but it seems not good, so wonder your method.


You can open telnet sessions:

  -monitor tcp:0:,server,nowait,telnet \
  -monitor tcp:0:4445,server,nowait,telnet

But note that, besides testing, I'd like to see an explanation on
the commitlog as to why dropping cur_mon doesn't brake the current
code.



  Thanks for the info, it works now.
  After re-check the code in readline_completion(), I think current
code with cur_mon, is redundant:

mon = cur_mon;
...

} else {
monitor_printf(mon, \n);
...
 ...
for(i = 0; i  rs-nb_completions; i++) {
monitor_printf(rs-mon, %-*s, max_width, rs-completions[i]);
if (++j == nb_cols || i == (rs-nb_completions - 1)) {
monitor_printf(rs-mon, \n);
j = 0;
}
}
readline_show_prompt(rs);
}

  You can see later, it uses rs-mon to printf the auto completion
result, so the first 'monitor_printf(mon, \n);', is used to start
a new line for the completion result, so mon should always be equal
to rs-mon, I can't think out a case that one monitor instance does
not print tips in new line while another monitor instance does. I'll
add a few line for it in commit message.

  About pass cur_mon to readline_completion(), I think parameter *rs
already have member rs-mon, allowing specify *mon would specify
the encapsulation a bit.

--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

2013-07-09 Thread Wenchao Xia 



You could pass cur_mon to readline_completion() in readline_handle_byte()
to avoid all this, but it would be preferable to clarify the matter.

This is also another benefit of having readline_completion() in a
different patch, you can (and should!) clarify this point in the commit
log along with testing results.


   OK, I'll test multiple monitors case.




  Do you have an example about how invoke qemu with multiple monitor?
Do you have recommendation about the way to connect to qemu's moniotr?
I tried pty with minicom but it seems not good, so wonder your method.


--
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

2013-07-09 Thread Luiz Capitulino 
On Tue, 09 Jul 2013 22:03:42 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:

 
  You could pass cur_mon to readline_completion() in readline_handle_byte()
  to avoid all this, but it would be preferable to clarify the matter.
 
  This is also another benefit of having readline_completion() in a
  different patch, you can (and should!) clarify this point in the commit
  log along with testing results.
 
 OK, I'll test multiple monitors case.
 
 
 
Do you have an example about how invoke qemu with multiple monitor?
 Do you have recommendation about the way to connect to qemu's moniotr?
 I tried pty with minicom but it seems not good, so wonder your method.

You can open telnet sessions:

 -monitor tcp:0:,server,nowait,telnet \
 -monitor tcp:0:4445,server,nowait,telnet

But note that, besides testing, I'd like to see an explanation on
the commitlog as to why dropping cur_mon doesn't brake the current
code.

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

2013-07-08 Thread Luiz Capitulino 
On Sat, 29 Jun 2013 11:52:55 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:

 Parameter *mon is added to replace *cur_mon, and readline_completion()
 pass rs-mon as value, which should be initialized in readline_init()
 called by monitor_init(). In short, structure ReadLineState controls
 where the action would be taken now.

This patch could be made easier to review if you further split it.
I'd make the following split (each item is a separate patch):

 1. Convert cmd_completion()
 2. Convert file_completion()
 3. Convert block_completion_it()
 4. Convert monitor_find_completion()
 5. Convert readline_completion()

One more comment at the bottom.

 Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
 Reviewed-by: Eric Blake ebl...@redhat.com
 ---
  include/monitor/readline.h |3 +-
  monitor.c  |   55 ++-
  readline.c |5 +--
  3 files changed, 37 insertions(+), 26 deletions(-)
 
 diff --git a/include/monitor/readline.h b/include/monitor/readline.h
 index fc9806e..0faf6e1 100644
 --- a/include/monitor/readline.h
 +++ b/include/monitor/readline.h
 @@ -8,7 +8,8 @@
  #define READLINE_MAX_COMPLETIONS 256
  
  typedef void ReadLineFunc(Monitor *mon, const char *str, void *opaque);
 -typedef void ReadLineCompletionFunc(const char *cmdline);
 +typedef void ReadLineCompletionFunc(Monitor *mon,
 +const char *cmdline);
  
  typedef struct ReadLineState {
  char cmd_buf[READLINE_CMD_BUF_SIZE + 1];
 diff --git a/monitor.c b/monitor.c
 index 9be515c..29e3a95 100644
 --- a/monitor.c
 +++ b/monitor.c
 @@ -3998,7 +3998,7 @@ out:
  QDECREF(qdict);
  }
  
 -static void cmd_completion(const char *name, const char *list)
 +static void cmd_completion(Monitor *mon, const char *name, const char *list)
  {
  const char *p, *pstart;
  char cmd[128];
 @@ -4016,7 +4016,7 @@ static void cmd_completion(const char *name, const char 
 *list)
  memcpy(cmd, pstart, len);
  cmd[len] = '\0';
  if (name[0] == '\0' || !strncmp(name, cmd, strlen(name))) {
 -readline_add_completion(cur_mon-rs, cmd);
 +readline_add_completion(mon-rs, cmd);
  }
  if (*p == '\0')
  break;
 @@ -4024,7 +4024,7 @@ static void cmd_completion(const char *name, const char 
 *list)
  }
  }
  
 -static void file_completion(const char *input)
 +static void file_completion(Monitor *mon, const char *input)
  {
  DIR *ffs;
  struct dirent *d;
 @@ -4047,7 +4047,7 @@ static void file_completion(const char *input)
  pstrcpy(file_prefix, sizeof(file_prefix), p + 1);
  }
  #ifdef DEBUG_COMPLETION
 -monitor_printf(cur_mon, input='%s' path='%s' prefix='%s'\n,
 +monitor_printf(mon, input='%s' path='%s' prefix='%s'\n,
 input, path, file_prefix);
  #endif
  ffs = opendir(path);
 @@ -4074,20 +4074,27 @@ static void file_completion(const char *input)
  if (stat(file, sb) == 0  S_ISDIR(sb.st_mode)) {
  pstrcat(file, sizeof(file), /);
  }
 -readline_add_completion(cur_mon-rs, file);
 +readline_add_completion(mon-rs, file);
  }
  }
  closedir(ffs);
  }
  
 +typedef struct MonitorBlockComplete {
 +Monitor *mon;
 +const char *input;
 +} MonitorBlockComplete;
 +
  static void block_completion_it(void *opaque, BlockDriverState *bs)
  {
  const char *name = bdrv_get_device_name(bs);
 -const char *input = opaque;
 +MonitorBlockComplete *mbc = opaque;
 +Monitor *mon = mbc-mon;
 +const char *input = mbc-input;
  
  if (input[0] == '\0' ||
  !strncmp(name, (char *)input, strlen(input))) {
 -readline_add_completion(cur_mon-rs, name);
 +readline_add_completion(mon-rs, name);
  }
  }
  
 @@ -4123,18 +4130,20 @@ static const char *next_arg_type(const char *typestr)
  return (p != NULL ? ++p : typestr);
  }
  
 -static void monitor_find_completion(const char *cmdline)
 +static void monitor_find_completion(Monitor *mon,
 +const char *cmdline)
  {
  const char *cmdname;
  char *args[MAX_ARGS];
  int nb_args, i, len;
  const char *ptype, *str;
  const mon_cmd_t *cmd;
 +MonitorBlockComplete mbs;
  
  parse_cmdline(cmdline, nb_args, args);
  #ifdef DEBUG_COMPLETION
 -for(i = 0; i  nb_args; i++) {
 -monitor_printf(cur_mon, arg%d = '%s'\n, i, (char *)args[i]);
 +for (i = 0; i  nb_args; i++) {
 +monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
  }
  #endif
  
 @@ -4153,9 +4162,9 @@ static void monitor_find_completion(const char *cmdline)
  cmdname = ;
  else
  cmdname = args[0];
 -readline_set_completion_index(cur_mon-rs, strlen(cmdname));
 +readline_set_completion_index(mon-rs, strlen(cmdname));
  for(cmd = mon_cmds; cmd-name != NULL; cmd++) {
 -

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

2013-07-08 Thread Wenchao Xia 

于 2013-7-8 23:17, Luiz Capitulino 写道:

On Sat, 29 Jun 2013 11:52:55 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:


Parameter *mon is added to replace *cur_mon, and readline_completion()
pass rs-mon as value, which should be initialized in readline_init()
called by monitor_init(). In short, structure ReadLineState controls
where the action would be taken now.


This patch could be made easier to review if you further split it.
I'd make the following split (each item is a separate patch):

  1. Convert cmd_completion()
  2. Convert file_completion()
  3. Convert block_completion_it()
  4. Convert monitor_find_completion()
  5. Convert readline_completion()

One more comment at the bottom.


  I'll split it as above.


Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
  include/monitor/readline.h |3 +-
  monitor.c  |   55 ++-
  readline.c |5 +--
  3 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/include/monitor/readline.h b/include/monitor/readline.h
index fc9806e..0faf6e1 100644
--- a/include/monitor/readline.h
+++ b/include/monitor/readline.h
@@ -8,7 +8,8 @@
  #define READLINE_MAX_COMPLETIONS 256

  typedef void ReadLineFunc(Monitor *mon, const char *str, void *opaque);
-typedef void ReadLineCompletionFunc(const char *cmdline);
+typedef void ReadLineCompletionFunc(Monitor *mon,
+const char *cmdline);

  typedef struct ReadLineState {
  char cmd_buf[READLINE_CMD_BUF_SIZE + 1];
diff --git a/monitor.c b/monitor.c
index 9be515c..29e3a95 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3998,7 +3998,7 @@ out:
  QDECREF(qdict);
  }

-static void cmd_completion(const char *name, const char *list)
+static void cmd_completion(Monitor *mon, const char *name, const char *list)
  {
  const char *p, *pstart;
  char cmd[128];
@@ -4016,7 +4016,7 @@ static void cmd_completion(const char *name, const char 
*list)
  memcpy(cmd, pstart, len);
  cmd[len] = '\0';
  if (name[0] == '\0' || !strncmp(name, cmd, strlen(name))) {
-readline_add_completion(cur_mon-rs, cmd);
+readline_add_completion(mon-rs, cmd);
  }
  if (*p == '\0')
  break;
@@ -4024,7 +4024,7 @@ static void cmd_completion(const char *name, const char 
*list)
  }
  }

-static void file_completion(const char *input)
+static void file_completion(Monitor *mon, const char *input)
  {
  DIR *ffs;
  struct dirent *d;
@@ -4047,7 +4047,7 @@ static void file_completion(const char *input)
  pstrcpy(file_prefix, sizeof(file_prefix), p + 1);
  }
  #ifdef DEBUG_COMPLETION
-monitor_printf(cur_mon, input='%s' path='%s' prefix='%s'\n,
+monitor_printf(mon, input='%s' path='%s' prefix='%s'\n,
 input, path, file_prefix);
  #endif
  ffs = opendir(path);
@@ -4074,20 +4074,27 @@ static void file_completion(const char *input)
  if (stat(file, sb) == 0  S_ISDIR(sb.st_mode)) {
  pstrcat(file, sizeof(file), /);
  }
-readline_add_completion(cur_mon-rs, file);
+readline_add_completion(mon-rs, file);
  }
  }
  closedir(ffs);
  }

+typedef struct MonitorBlockComplete {
+Monitor *mon;
+const char *input;
+} MonitorBlockComplete;
+
  static void block_completion_it(void *opaque, BlockDriverState *bs)
  {
  const char *name = bdrv_get_device_name(bs);
-const char *input = opaque;
+MonitorBlockComplete *mbc = opaque;
+Monitor *mon = mbc-mon;
+const char *input = mbc-input;

  if (input[0] == '\0' ||
  !strncmp(name, (char *)input, strlen(input))) {
-readline_add_completion(cur_mon-rs, name);
+readline_add_completion(mon-rs, name);
  }
  }

@@ -4123,18 +4130,20 @@ static const char *next_arg_type(const char *typestr)
  return (p != NULL ? ++p : typestr);
  }

-static void monitor_find_completion(const char *cmdline)
+static void monitor_find_completion(Monitor *mon,
+const char *cmdline)
  {
  const char *cmdname;
  char *args[MAX_ARGS];
  int nb_args, i, len;
  const char *ptype, *str;
  const mon_cmd_t *cmd;
+MonitorBlockComplete mbs;

  parse_cmdline(cmdline, nb_args, args);
  #ifdef DEBUG_COMPLETION
-for(i = 0; i  nb_args; i++) {
-monitor_printf(cur_mon, arg%d = '%s'\n, i, (char *)args[i]);
+for (i = 0; i  nb_args; i++) {
+monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
  }
  #endif

@@ -4153,9 +4162,9 @@ static void monitor_find_completion(const char *cmdline)
  cmdname = ;
  else
  cmdname = args[0];
-readline_set_completion_index(cur_mon-rs, strlen(cmdname));
+readline_set_completion_index(mon-rs, strlen(cmdname));
  for(cmd = mon_cmds; cmd-name != NULL; cmd++) {
-

[Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

2013-06-28 Thread Wenchao Xia 
Parameter *mon is added to replace *cur_mon, and readline_completion()
pass rs-mon as value, which should be initialized in readline_init()
called by monitor_init(). In short, structure ReadLineState controls
where the action would be taken now.

Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
 include/monitor/readline.h |3 +-
 monitor.c  |   55 ++-
 readline.c |5 +--
 3 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/include/monitor/readline.h b/include/monitor/readline.h
index fc9806e..0faf6e1 100644
--- a/include/monitor/readline.h
+++ b/include/monitor/readline.h
@@ -8,7 +8,8 @@
 #define READLINE_MAX_COMPLETIONS 256
 
 typedef void ReadLineFunc(Monitor *mon, const char *str, void *opaque);
-typedef void ReadLineCompletionFunc(const char *cmdline);
+typedef void ReadLineCompletionFunc(Monitor *mon,
+const char *cmdline);
 
 typedef struct ReadLineState {
 char cmd_buf[READLINE_CMD_BUF_SIZE + 1];
diff --git a/monitor.c b/monitor.c
index 9be515c..29e3a95 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3998,7 +3998,7 @@ out:
 QDECREF(qdict);
 }
 
-static void cmd_completion(const char *name, const char *list)
+static void cmd_completion(Monitor *mon, const char *name, const char *list)
 {
 const char *p, *pstart;
 char cmd[128];
@@ -4016,7 +4016,7 @@ static void cmd_completion(const char *name, const char 
*list)
 memcpy(cmd, pstart, len);
 cmd[len] = '\0';
 if (name[0] == '\0' || !strncmp(name, cmd, strlen(name))) {
-readline_add_completion(cur_mon-rs, cmd);
+readline_add_completion(mon-rs, cmd);
 }
 if (*p == '\0')
 break;
@@ -4024,7 +4024,7 @@ static void cmd_completion(const char *name, const char 
*list)
 }
 }
 
-static void file_completion(const char *input)
+static void file_completion(Monitor *mon, const char *input)
 {
 DIR *ffs;
 struct dirent *d;
@@ -4047,7 +4047,7 @@ static void file_completion(const char *input)
 pstrcpy(file_prefix, sizeof(file_prefix), p + 1);
 }
 #ifdef DEBUG_COMPLETION
-monitor_printf(cur_mon, input='%s' path='%s' prefix='%s'\n,
+monitor_printf(mon, input='%s' path='%s' prefix='%s'\n,
input, path, file_prefix);
 #endif
 ffs = opendir(path);
@@ -4074,20 +4074,27 @@ static void file_completion(const char *input)
 if (stat(file, sb) == 0  S_ISDIR(sb.st_mode)) {
 pstrcat(file, sizeof(file), /);
 }
-readline_add_completion(cur_mon-rs, file);
+readline_add_completion(mon-rs, file);
 }
 }
 closedir(ffs);
 }
 
+typedef struct MonitorBlockComplete {
+Monitor *mon;
+const char *input;
+} MonitorBlockComplete;
+
 static void block_completion_it(void *opaque, BlockDriverState *bs)
 {
 const char *name = bdrv_get_device_name(bs);
-const char *input = opaque;
+MonitorBlockComplete *mbc = opaque;
+Monitor *mon = mbc-mon;
+const char *input = mbc-input;
 
 if (input[0] == '\0' ||
 !strncmp(name, (char *)input, strlen(input))) {
-readline_add_completion(cur_mon-rs, name);
+readline_add_completion(mon-rs, name);
 }
 }
 
@@ -4123,18 +4130,20 @@ static const char *next_arg_type(const char *typestr)
 return (p != NULL ? ++p : typestr);
 }
 
-static void monitor_find_completion(const char *cmdline)
+static void monitor_find_completion(Monitor *mon,
+const char *cmdline)
 {
 const char *cmdname;
 char *args[MAX_ARGS];
 int nb_args, i, len;
 const char *ptype, *str;
 const mon_cmd_t *cmd;
+MonitorBlockComplete mbs;
 
 parse_cmdline(cmdline, nb_args, args);
 #ifdef DEBUG_COMPLETION
-for(i = 0; i  nb_args; i++) {
-monitor_printf(cur_mon, arg%d = '%s'\n, i, (char *)args[i]);
+for (i = 0; i  nb_args; i++) {
+monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
 }
 #endif
 
@@ -4153,9 +4162,9 @@ static void monitor_find_completion(const char *cmdline)
 cmdname = ;
 else
 cmdname = args[0];
-readline_set_completion_index(cur_mon-rs, strlen(cmdname));
+readline_set_completion_index(mon-rs, strlen(cmdname));
 for(cmd = mon_cmds; cmd-name != NULL; cmd++) {
-cmd_completion(cmdname, cmd-name);
+cmd_completion(mon, cmdname, cmd-name);
 }
 } else {
 /* find the command */
@@ -4183,33 +4192,35 @@ static void monitor_find_completion(const char *cmdline)
 switch(*ptype) {
 case 'F':
 /* file completion */
-readline_set_completion_index(cur_mon-rs, strlen(str));
-file_completion(str);
+readline_set_completion_index(mon-rs, strlen(str));
+file_completion(mon, str);
 break;