[Qemu-devel] [PATCH V10 06/15] monitor: call sortcmdlist() only one time
It doesn't need to be done for every monitor, so change it.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
monitor.c |3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/monitor.c b/monitor.c
index fffb434..db4d441 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4762,6 +4762,7 @@ void monitor_init(CharDriverState *chr, int flags)
if (is_first_init) {
monitor_protocol_event_init();
+sortcmdlist();
is_first_init = 0;
}
@@ -4791,8 +4792,6 @@ void monitor_init(CharDriverState *chr, int flags)
QLIST_INSERT_HEAD(mon_list, mon, entry);
if (!default_mon || (flags MONITOR_IS_DEFAULT))
default_mon = mon;
-
-sortcmdlist();
}
static void bdrv_password_cb(Monitor *mon, const char *password, void *opaque)
--
1.7.1
[Qemu-devel] [PATCH V10 12/15] monitor: refine monitor_find_completion()
In order to support sub command in auto completion, a reentrant function
is needed, so monitor_find_completion() is split into two parts. The
first part does parsing of user input which need to be done only once,
the second part does the auto completion job according to the parsing
result, which contains the necessary code to support sub command and
works as the reentrant function. The global info_cmds is still used
in second part, which will be replaced by sub command code later.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 65
1 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/monitor.c b/monitor.c
index c8855d2..9d1b0cb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4229,34 +4229,17 @@ static const char *next_arg_type(const char *typestr)
return (p != NULL ? ++p : typestr);
}
-static void monitor_find_completion(Monitor *mon,
-const char *cmdline)
+static void monitor_find_completion_by_table(Monitor *mon,
+ const mon_cmd_t *cmd_table,
+ char **args,
+ int nb_args)
{
const char *cmdname;
-char *args[MAX_ARGS];
-int nb_args, i, len;
+int i;
const char *ptype, *str;
const mon_cmd_t *cmd;
MonitorBlockComplete mbs;
-if (parse_cmdline(cmdline, nb_args, args) 0) {
-return;
-}
-#ifdef DEBUG_COMPLETION
-for (i = 0; i nb_args; i++) {
-monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
-}
-#endif
-
-/* if the line ends with a space, it means we want to complete the
- next arg */
-len = strlen(cmdline);
-if (len 0 qemu_isspace(cmdline[len - 1])) {
-if (nb_args = MAX_ARGS) {
-goto cleanup;
-}
-args[nb_args++] = g_strdup();
-}
if (nb_args = 1) {
/* command completion */
if (nb_args == 0)
@@ -4264,18 +4247,18 @@ static void monitor_find_completion(Monitor *mon,
else
cmdname = args[0];
readline_set_completion_index(mon-rs, strlen(cmdname));
-for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
+for (cmd = cmd_table; cmd-name != NULL; cmd++) {
cmd_completion(mon, cmdname, cmd-name);
}
} else {
/* find the command */
-for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
+for (cmd = cmd_table; cmd-name != NULL; cmd++) {
if (compare_cmd(args[0], cmd-name)) {
break;
}
}
if (!cmd-name) {
-goto cleanup;
+return;
}
ptype = next_arg_type(cmd-args_type);
@@ -4320,7 +4303,7 @@ static void monitor_find_completion(Monitor *mon,
}
} else if (!strcmp(cmd-name, help|?)) {
readline_set_completion_index(mon-rs, strlen(str));
-for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
+for (cmd = cmd_table; cmd-name != NULL; cmd++) {
cmd_completion(mon, str, cmd-name);
}
}
@@ -4329,6 +4312,36 @@ static void monitor_find_completion(Monitor *mon,
break;
}
}
+}
+
+static void monitor_find_completion(Monitor *mon,
+const char *cmdline)
+{
+char *args[MAX_ARGS];
+int nb_args, len;
+
+/* 1. parse the cmdline */
+if (parse_cmdline(cmdline, nb_args, args) 0) {
+return;
+}
+#ifdef DEBUG_COMPLETION
+for (i = 0; i nb_args; i++) {
+monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
+}
+#endif
+
+/* if the line ends with a space, it means we want to complete the
+ next arg */
+len = strlen(cmdline);
+if (len 0 qemu_isspace(cmdline[len - 1])) {
+if (nb_args = MAX_ARGS) {
+goto cleanup;
+}
+args[nb_args++] = g_strdup();
+}
+
+/* 2. auto complete according to args */
+monitor_find_completion_by_table(mon, mon-cmd_table, args, nb_args);
cleanup:
free_cmdline_args(args, nb_args);
--
1.7.1
[Qemu-devel] [PATCH V10 04/15] monitor: avoid use of global *cur_mon in monitor_find_completion()
Parameter *mon is added, and local variable *mon added in previous patch
is removed. The caller readline_completion(), pass rs-mon as value, which
should be initialized in readline_init() called by monitor_init().
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
include/monitor/readline.h |3 ++-
monitor.c | 18 +-
readline.c |2 +-
3 files changed, 12 insertions(+), 11 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 d1a6bda..fffb434 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4140,20 +4140,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;
-Monitor *mon = cur_mon;
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
@@ -4172,7 +4172,7 @@ 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(mon, cmdname, cmd-name);
}
@@ -4202,7 +4202,7 @@ static void monitor_find_completion(const char *cmdline)
switch(*ptype) {
case 'F':
/* file completion */
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
file_completion(mon, str);
break;
case 'B':
@@ -4215,7 +4215,7 @@ static void monitor_find_completion(const char *cmdline)
case 's':
/* XXX: more generic ? */
if (!strcmp(cmd-name, info)) {
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
for(cmd = info_cmds; cmd-name != NULL; cmd++) {
cmd_completion(mon, str, cmd-name);
}
@@ -4223,12 +4223,12 @@ static void monitor_find_completion(const char *cmdline)
char *sep = strrchr(str, '-');
if (sep)
str = sep + 1;
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
for (i = 0; i Q_KEY_CODE_MAX; i++) {
cmd_completion(mon, str, QKeyCode_lookup[i]);
}
} else if (!strcmp(cmd-name, help|?)) {
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
for (cmd = mon_cmds; cmd-name != NULL; cmd++) {
cmd_completion(mon, str, cmd-name);
}
diff --git a/readline.c b/readline.c
index 1c0f7ee..c91b324 100644
--- a/readline.c
+++ b/readline.c
@@ -285,7 +285,7 @@ static void readline_completion(ReadLineState *rs)
cmdline = g_malloc(rs-cmd_buf_index + 1);
memcpy(cmdline, rs-cmd_buf, rs-cmd_buf_index);
cmdline[rs-cmd_buf_index] = '\0';
-rs-completion_finder(cmdline);
+rs-completion_finder(rs-mon, cmdline);
g_free(cmdline);
/* no completion found */
--
1.7.1
[Qemu-devel] [PATCH V10 02/15] monitor: avoid use of global *cur_mon in file_completion()
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/monitor.c b/monitor.c
index 31b527d..a59a402 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4034,7 +4034,7 @@ static void cmd_completion(Monitor *mon, 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;
@@ -4057,7 +4057,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);
@@ -4084,7 +4084,7 @@ 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);
@@ -4195,7 +4195,7 @@ static void monitor_find_completion(const char *cmdline)
case 'F':
/* file completion */
readline_set_completion_index(cur_mon-rs, strlen(str));
-file_completion(str);
+file_completion(mon, str);
break;
case 'B':
/* block device name completion */
--
1.7.1
[Qemu-devel] [PATCH V10 14/15] monitor: allow help show message for single command in sub group
A new parameter type 'S' is introduced to allow user input any string.
help info block works normal now.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
hmp-commands.hx |2 +-
monitor.c | 27 +++
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8c6b91a..c161fe9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -11,7 +11,7 @@ ETEXI
{
.name = help|?,
-.args_type = name:s?,
+.args_type = name:S?,
.params = [cmd],
.help = show the help,
.mhandler.cmd = do_help_cmd,
diff --git a/monitor.c b/monitor.c
index d3f357c..81f41e6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,6 +83,7 @@
* 'F' filename
* 'B' block device name
* 's' string (accept optional quote)
+ * 'S' it just appends the rest of the string (accept optional quote)
* 'O' option string of the form NAME=VALUE,...
* parsed according to QemuOptsList given by its name
* Example: 'device:O' uses qemu_device_opts.
@@ -4046,6 +4047,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor
*mon,
}
}
break;
+case 'S':
+{
+/* package all remaining string */
+int len;
+
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*typestr == '?') {
+typestr++;
+if (*p == '\0') {
+/* no remaining string: NULL argument */
+break;
+}
+}
+len = strlen(p);
+if (len = 0) {
+monitor_printf(mon, %s: string expected\n,
+ cmdname);
+break;
+}
+qdict_put(qdict, key, qstring_from_str(p));
+p += len;
+}
+break;
default:
bad_type:
monitor_printf(mon, %s: unknown type '%c'\n, cmdname, c);
@@ -4293,6 +4319,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
bdrv_iterate(block_completion_it, mbs);
break;
case 's':
+case 'S':
if (!strcmp(cmd-name, sendkey)) {
char *sep = strrchr(str, '-');
if (sep)
--
1.7.1
[Qemu-devel] [PATCH V10 07/15] monitor: split off monitor_data_init()
In qmp_human_monitor_command(), the monitor need to initialized for
basic functionalities, and later more init code will be added, so
split off this function. Note that it is different with QMP mode
monitor which accept json string from monitor's input,
qmp_human_monitor_command() retrieve the human style command from
QMP input, then send the command to a normal mode monitor.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
monitor.c | 20 +++-
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/monitor.c b/monitor.c
index db4d441..bd02112 100644
--- a/monitor.c
+++ b/monitor.c
@@ -683,14 +683,24 @@ static int do_qmp_capabilities(Monitor *mon, const QDict
*params,
static void handle_user_command(Monitor *mon, const char *cmdline);
+static void monitor_data_init(Monitor *mon)
+{
+memset(mon, 0, sizeof(Monitor));
+mon-outbuf = qstring_new();
+}
+
+static void monitor_data_destroy(Monitor *mon)
+{
+QDECREF(mon-outbuf);
+}
+
char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
int64_t cpu_index, Error **errp)
{
char *output = NULL;
Monitor *old_mon, hmp;
-memset(hmp, 0, sizeof(hmp));
-hmp.outbuf = qstring_new();
+monitor_data_init(hmp);
hmp.skip_flush = true;
old_mon = cur_mon;
@@ -716,7 +726,7 @@ char *qmp_human_monitor_command(const char *command_line,
bool has_cpu_index,
}
out:
-QDECREF(hmp.outbuf);
+monitor_data_destroy(hmp);
return output;
}
@@ -4766,8 +4776,8 @@ void monitor_init(CharDriverState *chr, int flags)
is_first_init = 0;
}
-mon = g_malloc0(sizeof(*mon));
-mon-outbuf = qstring_new();
+mon = g_malloc(sizeof(*mon));
+monitor_data_init(mon);
mon-chr = chr;
mon-flags = flags;
--
1.7.1
[Qemu-devel] [PATCH V10 11/15] monitor: support sub command in help
The old code in help_cmd() uses global 'info_cmds' and treats it as a
special case. Actually 'info_cmds' is a sub command group of 'mon_cmds',
in order to avoid direct use of it, help_cmd() needs to change its work
mechanism to support sub command and not treat it as a special case
any more.
To support sub command, help_cmd() will first parse the input and then call
help_cmd_dump(), which works as a reentrant function. When it meets a sub
command, it simply enters the function again. Since help dumping needs to
know whole input to printf full help message include prefix, for example,
help info block need to printf prefix info, so help_cmd_dump() takes all
args from input and extra parameter arg_index to identify the progress.
Another function help_cmd_dump_one() is introduced to printf the prefix
and command's help message.
Now help supports sub command, so later if another sub command group is
added in any depth, help will automatically work for it. Still help info
block will show error since command parser reject additional parameter,
which can be improved later. log is still treated as a special case.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
monitor.c | 62 +++-
1 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/monitor.c b/monitor.c
index 2a9dfb2..c8855d2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -880,33 +880,75 @@ static int parse_cmdline(const char *cmdline,
return -1;
}
+static void help_cmd_dump_one(Monitor *mon,
+ const mon_cmd_t *cmd,
+ char **prefix_args,
+ int prefix_args_nb)
+{
+int i;
+
+for (i = 0; i prefix_args_nb; i++) {
+monitor_printf(mon, %s , prefix_args[i]);
+}
+monitor_printf(mon, %s %s -- %s\n, cmd-name, cmd-params, cmd-help);
+}
+
+/* @args[@arg_index] is the valid command need to find in @cmds */
static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
- const char *prefix, const char *name)
+ char **args, int nb_args, int arg_index)
{
const mon_cmd_t *cmd;
-for(cmd = cmds; cmd-name != NULL; cmd++) {
-if (!name || !strcmp(name, cmd-name))
-monitor_printf(mon, %s%s %s -- %s\n, prefix, cmd-name,
- cmd-params, cmd-help);
+/* No valid arg need to compare with, dump all in *cmds */
+if (arg_index = nb_args) {
+for (cmd = cmds; cmd-name != NULL; cmd++) {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+return;
+}
+
+/* Find one entry to dump */
+for (cmd = cmds; cmd-name != NULL; cmd++) {
+if (compare_cmd(args[arg_index], cmd-name)) {
+if (cmd-sub_table) {
+/* continue with next arg */
+help_cmd_dump(mon, cmd-sub_table,
+ args, nb_args, arg_index + 1);
+} else {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+break;
+}
}
}
static void help_cmd(Monitor *mon, const char *name)
{
-if (name !strcmp(name, info)) {
-help_cmd_dump(mon, info_cmds, info , NULL);
-} else {
-help_cmd_dump(mon, mon-cmd_table, , name);
-if (name !strcmp(name, log)) {
+char *args[MAX_ARGS];
+int nb_args = 0;
+
+/* 1. parse user input */
+if (name) {
+/* special case for log, directly dump and return */
+if (!strcmp(name, log)) {
const QEMULogItem *item;
monitor_printf(mon, Log items (comma separated):\n);
monitor_printf(mon, %-10s %s\n, none, remove all logs);
for (item = qemu_log_items; item-mask != 0; item++) {
monitor_printf(mon, %-10s %s\n, item-name, item-help);
}
+return;
+}
+
+if (parse_cmdline(name, nb_args, args) 0) {
+return;
}
}
+
+/* 2. dump the contents according to parsed args */
+help_cmd_dump(mon, mon-cmd_table, args, nb_args, 0);
+
+free_cmdline_args(args, nb_args);
}
static void do_help_cmd(Monitor *mon, const QDict *qdict)
--
1.7.1
[Qemu-devel] [PATCH V10 15/15] monitor: improve auto complete of help for single command in sub group
Now special case help * in auto completion can work with sub commands,
such as help info u*.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/monitor.c b/monitor.c
index 81f41e6..8d07f9c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4329,10 +4329,8 @@ static void monitor_find_completion_by_table(Monitor
*mon,
cmd_completion(mon, str, QKeyCode_lookup[i]);
}
} else if (!strcmp(cmd-name, help|?)) {
-readline_set_completion_index(mon-rs, strlen(str));
-for (cmd = cmd_table; cmd-name != NULL; cmd++) {
-cmd_completion(mon, str, cmd-name);
-}
+monitor_find_completion_by_table(mon, cmd_table,
+ args[1], nb_args - 1);
}
break;
default:
--
1.7.1
[Qemu-devel] [PATCH V10 10/15] monitor: refine parse_cmdline()
Since this function will be used by help_cmd() later, so improve
it to make it more generic and easier to use. free_cmdline_args()
is added too as paired function to free the result.
One change of this function is that, when the valid args in input
exceed the limit of MAX_ARGS, it fails now, instead of return with
MAX_ARGS of parsed args in old code. This should not impact much
since it is rare that user input many args in monitor's help and
auto complete scenario.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 51 ---
1 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/monitor.c b/monitor.c
index 46191d3..2a9dfb2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -821,9 +821,33 @@ static int get_str(char *buf, int buf_size, const char
**pp)
#define MAX_ARGS 16
-/* NOTE: this parser is an approximate form of the real command parser */
-static void parse_cmdline(const char *cmdline,
- int *pnb_args, char **args)
+static void free_cmdline_args(char **args, int nb_args)
+{
+int i;
+
+assert(nb_args = MAX_ARGS);
+
+for (i = 0; i nb_args; i++) {
+g_free(args[i]);
+}
+
+}
+
+/*
+ * Parse the command line to get valid args.
+ * @cmdline: command line to be parsed.
+ * @pnb_args: location to store the number of args, must NOT be NULL.
+ * @args: location to store the args, which should be freed by caller, must
+ *NOT be NULL.
+ *
+ * Returns 0 on success, negative on failure.
+ *
+ * NOTE: this parser is an approximate form of the real command parser. Number
+ * of args have a limit of MAX_ARGS. If cmdline contains more, it will
+ * return with failure.
+ */
+static int parse_cmdline(const char *cmdline,
+ int *pnb_args, char **args)
{
const char *p;
int nb_args, ret;
@@ -839,16 +863,21 @@ static void parse_cmdline(const char *cmdline,
break;
}
if (nb_args = MAX_ARGS) {
-break;
+goto fail;
}
ret = get_str(buf, sizeof(buf), p);
-args[nb_args] = g_strdup(buf);
-nb_args++;
if (ret 0) {
-break;
+goto fail;
}
+args[nb_args] = g_strdup(buf);
+nb_args++;
}
*pnb_args = nb_args;
+return 0;
+
+ fail:
+free_cmdline_args(args, nb_args);
+return -1;
}
static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
@@ -4168,7 +4197,9 @@ static void monitor_find_completion(Monitor *mon,
const mon_cmd_t *cmd;
MonitorBlockComplete mbs;
-parse_cmdline(cmdline, nb_args, args);
+if (parse_cmdline(cmdline, nb_args, args) 0) {
+return;
+}
#ifdef DEBUG_COMPLETION
for (i = 0; i nb_args; i++) {
monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
@@ -4258,9 +4289,7 @@ static void monitor_find_completion(Monitor *mon,
}
cleanup:
-for (i = 0; i nb_args; i++) {
-g_free(args[i]);
-}
+free_cmdline_args(args, nb_args);
}
static int monitor_can_read(void *opaque)
--
1.7.1
Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
于 2013-8-27 9:43, Wenchao Xia 写道: 于 2013-8-26 23:55, Luiz Capitulino 写道: On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: This series make auto completion and help functions works normal for sub command, by using reentrant functions. In order to do that, global variables are not directly used in those functions any more. With this series, cmd_table is a member of structure Monitor so it is possible to create a monitor with different command table now, auto completion will work in that monitor. In short, info is not treated as a special case now, this series ensure help and auto complete function works normal for any sub command added in the future. Patch 5 replaced cur_mon with rs-mon, it is safe because: monitor_init() calls readline_init() which initialize mon-rs, result is mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make monitor_read() function take *mon as its opaque. Later, when user input, monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque. If qemu's monitors run in one thread, then later in readline_handle_byte() and readline_comletion(), cur_mon is actually equal to rs-mon, in another word, it points to the monitor instance, so it is safe to replace *cur_mon in those functions. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2 Sorry for it, I missed that CC flag in my configure, will fix. It would be great if you can share your configure settings. Respined v10 and tested with -Werror=unused-label, hope no other issue is missed in my test. -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V2 1/3] qemu-iotests: add unix socket help program
于 2013-8-28 9:11, Eric Blake 写道:
On 08/26/2013 08:52 PM, Wenchao Xia wrote:
This program can do a sendmsg call to transfer fd with unix
socket, which is not supported in python2.
The built binary will not be deleted in clean, but it is a
existing issue in ./tests, which should be solved in another
patch.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
+++ b/tests/qemu-iotests/socket_scm_helper.c
@@ -0,0 +1,119 @@
+/*
+ * SCM_RIGHT with unix socket help program for test
s/SCM_RIGHT/S/
+
+static int send_fd(int fd, int fd_to_send, const char *data, size_t len)
+{
+struct msghdr msg;
+struct iovec iov[1];
+int ret;
+char control[CMSG_SPACE(sizeof(int))];
+struct cmsghdr *cmsg;
+
+if (fd 0) {
+fprintf(stderr, Socket fd is invalid.\n);
+return -1;
+}
+if (fd_to_send 0) {
+fprintf(stderr, Fd to send is invalid.\n);
+return -1;
+}
+if (data == NULL || len = 0) {
len cannot be 0, since it is size_t (or did you mean for it to be
ssize_t?)
+if (ret 0) {
+fprintf(stderr, Failed to send msg, reason: %s.\n, strerror(errno));
Messages typically shouldn't end with '.'; especially when ending the
message with strerror.
+}
+
+return ret;
+}
+
+/*
+ * To make things simple, the caller need to specify:
s/need/needs/
+ * 1. socket fd.
+ * 2. fd to send.
+ * 3. msg to send.
+ */
+int main(int argc, char **argv, char **envp)
+{
+int sock, fd, ret, buflen;
+const char *buf;
+#ifdef SOCKET_SCM_DEBUG
+int i;
+for (i = 0; i argc; i++) {
+fprintf(stderr, Parameter %d: %s.\n, i, argv[i]);
Another useless ending '.' (and elsewhere throughout the patch)
+}
+#endif
+
+if (argc 4) {
+fprintf(stderr,
+Invalid parameter, use it as:\n
+%s SOCKET_FD FD_TO_SEND MSG.\n,
+argv[0]);
This rejects too few, but not too many, arguments. Should the condition
be: if (argc != 4)
+return 1;
I prefer EXIT_FAILURE over the magic number 1 (multiple instances).
+}
+
+errno = 0;
+sock = strtol(argv[1], NULL, 10);
Failure to pass in a second argument means you cannot distinguish
from 0 from 0a - all three input strings for argv[1] result in
sock==0 without you knowing any better. If you're going to use
strtol(), use it correctly; if you don't care about garbage, then atoi()
is just as (in)correct and with less typing.
I will check error more carefully with other issues addressed in
next version. Since the build system is modified, I'd like to wait a
few days to see if more comment comes.
+if (errno) {
+fprintf(stderr, Failed in strtol for socket fd, reason: %s.\n,
+strerror(errno));
+return 1;
+}
+fd = strtol(argv[2], NULL, 10);
+if (errno) {
+fprintf(stderr, Failed in strtol for fd to send, reason: %s.\n,
+strerror(errno));
+return 1;
+}
+
+buf = argv[3];
+buflen = strlen(buf);
+
+ret = send_fd(sock, fd, buf, buflen);
+if (ret 0) {
+return 1;
+}
+return 0;
I prefer EXIT_SUCCESS over the magic number 0.
+}
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
于 2013-8-27 21:19, Luiz Capitulino 写道: On Tue, 27 Aug 2013 20:39:37 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-27 9:43, Wenchao Xia 写道: 于 2013-8-26 23:55, Luiz Capitulino 写道: On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: This series make auto completion and help functions works normal for sub command, by using reentrant functions. In order to do that, global variables are not directly used in those functions any more. With this series, cmd_table is a member of structure Monitor so it is possible to create a monitor with different command table now, auto completion will work in that monitor. In short, info is not treated as a special case now, this series ensure help and auto complete function works normal for any sub command added in the future. Patch 5 replaced cur_mon with rs-mon, it is safe because: monitor_init() calls readline_init() which initialize mon-rs, result is mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make monitor_read() function take *mon as its opaque. Later, when user input, monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque. If qemu's monitors run in one thread, then later in readline_handle_byte() and readline_comletion(), cur_mon is actually equal to rs-mon, in another word, it points to the monitor instance, so it is safe to replace *cur_mon in those functions. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2 Sorry for it, I missed that CC flag in my configure, will fix. It would be great if you can share your configure settings. Respined v10 and tested with -Werror=unused-label, hope no other issue is missed in my test. My configure line doesn't have anything special: ../configure --target-list=x86_64-softmmu I used special cc warn flag before, it seems that by default more flags would be set, will double check with default configure. -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V8 00/13] monitor: support sub command group in auto completion and help
于 2013-8-22 21:12, Luiz Capitulino 写道:
On Thu, 22 Aug 2013 17:16:23 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
于 2013-8-20 22:04, Luiz Capitulino 写道:
On Tue, 30 Jul 2013 12:03:11 -0400
Luiz Capitulino lcapitul...@redhat.com wrote:
On Fri, 26 Jul 2013 11:20:29 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
This series make auto completion and help functions works normal for sub
command, by using reentrant functions. In order to do that, global variables
are not directly used in those functions any more. With this series, cmd_table
is a member of structure Monitor so it is possible to create a monitor with
different command table now, auto completion will work in that monitor. In
short, info is not treated as a special case now, this series ensure help
and auto complete function works normal for any sub command added in the future.
Patch 5 replaced cur_mon with rs-mon, it is safe because:
monitor_init() calls readline_init() which initialize mon-rs, result is
mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make
monitor_read() function take *mon as its opaque. Later, when user input,
monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque.
If qemu's monitors run in one thread, then later in readline_handle_byte()
and readline_comletion(), cur_mon is actually equal to rs-mon, in another
word, it points to the monitor instance, so it is safe to replace *cur_mon
in those functions.
I've applied this to qmp-next with the change I suggested for
patch 09/13.
Unfortunately this series brakes make check:
GTESTER check-qtest-x86_64
Broken pipe
GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
main-loop: WARNING: I/O thread spun for 1000 iterations
make: *** [check-qtest-x86_64] Error 1
I debugged it (with some help from Laszlo) and the problem is that it
broke the human-monitor-command command. Any usage of this command
triggers the bug like:
{ execute: human-monitor-command,
arguments: { command-line: info registers } }
It seems simple to fix, I think you just have to initialize
mon-cmd_table in qmp_human_monitor_command(), but I'd recommend two
things:
1. It's better to split off some/all QMP initialization from
monitor_init() and call it from qmp_human_monitor_command()
2. Can you please take the opportunity and test all commands using
cur_mon? Just grep for it
Sorry for noticing this only now, but I only run make check before
sending a pull request (although this very likely shows you didn't
run it either).
About the fd related qmp interface, to test it, send_msg() is needed,
which was not supported in python 2, but new added python 3.3. I think
there are three ways to add test cases for fd qmp APIs:
1 test only when python 3.3.
2 python plus C: compile a .so and call it with ctypes.
3 a new test framework: pure C code to call qmp interfaces.
Which one do you prefer?
Can't we have a C program plus a shell script to test this? Anyway, if
this gets complicated you can skip having the test-case. This series took
a long way already and holding it because of that test-case isn't fair.
C program can work, I tested it with currently qemu-iotest infra,
when fork() the fd wouldn't be closed in child process, so the child
C program can use the fds openned in parent python program. With shell
script I need to build up some basic infra such as vm boot up, monitor
connect, json parsing, so used qemu-iotest python infra instead of shell
script.
Based on that, I have sent a separate series to add the test case.
http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg03978.html
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [RFC] aio: add aio_context_acquire() and aio_context_release()
void test_notify(void)
g_assert(!aio_poll(ctx, false));
}
+typedef struct {
+QemuMutex start_lock;
+bool thread_acquired;
+} AcquireTestData;
+
+static void *test_acquire_thread(void *opaque)
+{
+AcquireTestData *data = opaque;
+
+/* Wait for other thread to let us start */
+qemu_mutex_lock(data-start_lock);
+qemu_mutex_unlock(data-start_lock);
+
+aio_context_acquire(ctx);
+aio_context_release(ctx);
+
+data-thread_acquired = true; /* success, we got here */
+
+return NULL;
+}
+
+static void dummy_notifier_read(EventNotifier *unused)
+{
+g_assert(false); /* should never be invoked */
+}
+
+static void test_acquire(void)
+{
+QemuThread thread;
+EventNotifier notifier;
+AcquireTestData data;
+
+/* Dummy event notifier ensures aio_poll() will block */
+event_notifier_init(notifier, false);
+aio_set_event_notifier(ctx, notifier, dummy_notifier_read);
+g_assert(!aio_poll(ctx, false)); /* consume aio_notify() */
+
+qemu_mutex_init(data.start_lock);
+qemu_mutex_lock(data.start_lock);
+data.thread_acquired = false;
+
+qemu_thread_create(thread, test_acquire_thread,
+ data, QEMU_THREAD_JOINABLE);
+
+/* Block in aio_poll(), let other thread kick us and acquire context */
+aio_context_acquire(ctx);
+qemu_mutex_unlock(data.start_lock); /* let the thread run */
+g_assert(!aio_poll(ctx, true));
+aio_context_release(ctx);
+
+qemu_thread_join(thread);
+aio_set_event_notifier(ctx, notifier, NULL);
+event_notifier_cleanup(notifier);
+
+g_assert(data.thread_acquired);
+}
+
static void test_bh_schedule(void)
{
BHTestData data = { .n = 0 };
@@ -639,6 +696,7 @@ int main(int argc, char **argv)
g_test_init(argc, argv, NULL);
g_test_add_func(/aio/notify, test_notify);
+g_test_add_func(/aio/acquire, test_acquire);
g_test_add_func(/aio/bh/schedule, test_bh_schedule);
g_test_add_func(/aio/bh/schedule10, test_bh_schedule10);
g_test_add_func(/aio/bh/cancel, test_bh_cancel);
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [RFC] aio: add aio_context_acquire() and aio_context_release()
*/
+qemu_mutex_lock(data-start_lock);
+qemu_mutex_unlock(data-start_lock);
+
+aio_context_acquire(ctx);
+aio_context_release(ctx);
+
+data-thread_acquired = true; /* success, we got here */
+
+return NULL;
+}
+
+static void dummy_notifier_read(EventNotifier *unused)
+{
+g_assert(false); /* should never be invoked */
+}
+
+static void test_acquire(void)
+{
+QemuThread thread;
+EventNotifier notifier;
+AcquireTestData data;
+
+/* Dummy event notifier ensures aio_poll() will block */
+event_notifier_init(notifier, false);
+aio_set_event_notifier(ctx, notifier, dummy_notifier_read);
+g_assert(!aio_poll(ctx, false)); /* consume aio_notify() */
+
+qemu_mutex_init(data.start_lock);
+qemu_mutex_lock(data.start_lock);
+data.thread_acquired = false;
+
+qemu_thread_create(thread, test_acquire_thread,
+ data, QEMU_THREAD_JOINABLE);
+
+/* Block in aio_poll(), let other thread kick us and acquire context */
+aio_context_acquire(ctx);
+qemu_mutex_unlock(data.start_lock); /* let the thread run */
+g_assert(!aio_poll(ctx, true));
+aio_context_release(ctx);
+
+qemu_thread_join(thread);
+aio_set_event_notifier(ctx, notifier, NULL);
+event_notifier_cleanup(notifier);
+
+g_assert(data.thread_acquired);
+}
+
static void test_bh_schedule(void)
{
BHTestData data = { .n = 0 };
@@ -639,6 +696,7 @@ int main(int argc, char **argv)
g_test_init(argc, argv, NULL);
g_test_add_func(/aio/notify, test_notify);
+g_test_add_func(/aio/acquire, test_acquire);
g_test_add_func(/aio/bh/schedule, test_bh_schedule);
g_test_add_func(/aio/bh/schedule10, test_bh_schedule10);
g_test_add_func(/aio/bh/cancel, test_bh_cancel);
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH 03/16] ipack: Pass size to ipack_bus_new_inplace()
To be passed to qbus_create_inplace().
Simplify DEVICE() cast to avoid parent field access.
s-dev will always point to pci_dev, so this change is safe.
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/char/ipack.c | 3 ++-
hw/char/ipack.h | 3 ++-
hw/char/tpci200.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/char/ipack.c b/hw/char/ipack.c
index f890471..5fb7073 100644
--- a/hw/char/ipack.c
+++ b/hw/char/ipack.c
@@ -24,7 +24,8 @@ IPackDevice *ipack_device_find(IPackBus *bus, int32_t slot)
return NULL;
}
-void ipack_bus_new_inplace(IPackBus *bus, DeviceState *parent,
+void ipack_bus_new_inplace(IPackBus *bus, size_t bus_size,
+ DeviceState *parent,
const char *name, uint8_t n_slots,
qemu_irq_handler handler)
{
diff --git a/hw/char/ipack.h b/hw/char/ipack.h
index f2b7a12..f8dc0f2 100644
--- a/hw/char/ipack.h
+++ b/hw/char/ipack.h
@@ -72,7 +72,8 @@ extern const VMStateDescription vmstate_ipack_device;
VMSTATE_STRUCT(_field, _state, 1, vmstate_ipack_device, IPackDevice)
IPackDevice *ipack_device_find(IPackBus *bus, int32_t slot);
-void ipack_bus_new_inplace(IPackBus *bus, DeviceState *parent,
+void ipack_bus_new_inplace(IPackBus *bus, size_t bus_size,
+ DeviceState *parent,
const char *name, uint8_t n_slots,
qemu_irq_handler handler);
diff --git a/hw/char/tpci200.c b/hw/char/tpci200.c
index d9e17b2..e04ff26 100644
--- a/hw/char/tpci200.c
+++ b/hw/char/tpci200.c
@@ -607,7 +607,7 @@ static int tpci200_initfn(PCIDevice *pci_dev)
pci_register_bar(s-dev, 4, PCI_BASE_ADDRESS_SPACE_MEMORY, s-las2);
pci_register_bar(s-dev, 5, PCI_BASE_ADDRESS_SPACE_MEMORY, s-las3);
-ipack_bus_new_inplace(s-bus, DEVICE(s-dev), NULL,
+ipack_bus_new_inplace(s-bus, sizeof(s-bus), DEVICE(pci_dev), NULL,
N_MODULES, tpci200_set_irq);
return 0;
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH 12/16] qdev: Pass size to qbus_create_inplace()
--git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 2233c54..8c7a61e 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -1309,7 +1309,8 @@ static int ccid_initfn(USBDevice *dev) usb_desc_create_serial(dev); usb_desc_init(dev); -qbus_create_inplace(s-bus.qbus, TYPE_CCID_BUS, dev-qdev, NULL); +qbus_create_inplace(s-bus, sizeof(s-bus), TYPE_CCID_BUS, DEVICE(dev), +NULL); s-intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP); s-bus.qbus.allow_hotplug = 1; s-card = NULL; diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c index 692979e..29cf284 100644 --- a/hw/virtio/virtio-mmio.c +++ b/hw/virtio/virtio-mmio.c @@ -392,7 +392,7 @@ static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size, DeviceState *qdev = DEVICE(dev); BusState *qbus; -qbus_create_inplace((BusState *)bus, TYPE_VIRTIO_MMIO_BUS, qdev, NULL); +qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_MMIO_BUS, qdev, NULL); qbus = BUS(bus); qbus-allow_hotplug = 0; } diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 313723f..a9a1893 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1512,7 +1512,7 @@ static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size, BusState *qbus; char virtio_bus_name[] = virtio-bus; -qbus_create_inplace((BusState *)bus, TYPE_VIRTIO_PCI_BUS, qdev, +qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_PCI_BUS, qdev, virtio_bus_name); qbus = BUS(bus); qbus-allow_hotplug = 1; diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 46972f4..a62f231 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -264,7 +264,7 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id); typedef int (qbus_walkerfn)(BusState *bus, void *opaque); typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque); -void qbus_create_inplace(void *bus, const char *typename, +void qbus_create_inplace(void *bus, size_t size, const char *typename, DeviceState *parent, const char *name); BusState *qbus_create(const char *typename, DeviceState *parent, const char *name); /* Returns 0 if either devfn or busfn skip walk somewhere in cursion, -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH 15/16] qdev-monitor: Clean up qdev_device_add() variable naming
于 2013-8-24 8:00, Andreas Färber 写道:
Avoid confusion between object and object class.
between object class and device class?
Tidy DeviceClass variable while at it.
Signed-off-by: Andreas Färber afaer...@suse.de
---
qdev-monitor.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 410cdcb..51bfec0 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -444,8 +444,8 @@ static BusState *qbus_find(const char *path)
DeviceState *qdev_device_add(QemuOpts *opts)
{
-ObjectClass *obj;
-DeviceClass *k;
+ObjectClass *oc;
+DeviceClass *dc;
const char *driver, *path, *id;
DeviceState *qdev;
BusState *bus = NULL;
@@ -457,22 +457,22 @@ DeviceState *qdev_device_add(QemuOpts *opts)
}
/* find driver */
-obj = object_class_by_name(driver);
-if (!obj) {
+oc = object_class_by_name(driver);
+if (!oc) {
const char *typename = find_typename_by_alias(driver);
if (typename) {
driver = typename;
-obj = object_class_by_name(driver);
+oc = object_class_by_name(driver);
}
}
-if (!obj) {
+if (!oc) {
qerror_report(QERR_INVALID_PARAMETER_VALUE, driver, device type);
return NULL;
}
-k = DEVICE_CLASS(obj);
+dc = DEVICE_CLASS(oc);
/* find bus */
path = qemu_opt_get(opts, bus);
@@ -481,16 +481,16 @@ DeviceState *qdev_device_add(QemuOpts *opts)
if (!bus) {
return NULL;
}
-if (!object_dynamic_cast(OBJECT(bus), k-bus_type)) {
+if (!object_dynamic_cast(OBJECT(bus), dc-bus_type)) {
qerror_report(QERR_BAD_BUS_FOR_DEVICE,
driver, object_get_typename(OBJECT(bus)));
return NULL;
}
-} else if (k-bus_type != NULL) {
-bus = qbus_find_recursive(sysbus_get_default(), NULL, k-bus_type);
+} else if (dc-bus_type != NULL) {
+bus = qbus_find_recursive(sysbus_get_default(), NULL, dc-bus_type);
if (!bus) {
qerror_report(QERR_NO_BUS_FOR_DEVICE,
- k-bus_type, driver);
+ dc-bus_type, driver);
return NULL;
}
}
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH 13/16] qom: Pass available size to object_initialize()
virtio_balloon_pci_instance_init(Object *obj)
{
VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
-object_initialize(dev-vdev, TYPE_VIRTIO_BALLOON);
+object_initialize(dev-vdev, sizeof(dev-vdev), TYPE_VIRTIO_BALLOON);
object_property_add_child(obj, virtio-backend, OBJECT(dev-vdev),
NULL);
object_property_add(obj, guest-stats, guest statistics,
@@ -1373,7 +1373,7 @@ static void virtio_serial_pci_class_init(ObjectClass
*klass, void *data)
static void virtio_serial_pci_instance_init(Object *obj)
{
VirtIOSerialPCI *dev = VIRTIO_SERIAL_PCI(obj);
-object_initialize(dev-vdev, TYPE_VIRTIO_SERIAL);
+object_initialize(dev-vdev, sizeof(dev-vdev), TYPE_VIRTIO_SERIAL);
object_property_add_child(obj, virtio-backend, OBJECT(dev-vdev),
NULL);
}
@@ -1432,7 +1432,7 @@ static void virtio_net_pci_class_init(ObjectClass *klass,
void *data)
static void virtio_net_pci_instance_init(Object *obj)
{
VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
-object_initialize(dev-vdev, TYPE_VIRTIO_NET);
+object_initialize(dev-vdev, sizeof(dev-vdev), TYPE_VIRTIO_NET);
object_property_add_child(obj, virtio-backend, OBJECT(dev-vdev),
NULL);
}
@@ -1488,7 +1488,7 @@ static void virtio_rng_pci_class_init(ObjectClass *klass,
void *data)
static void virtio_rng_initfn(Object *obj)
{
VirtIORngPCI *dev = VIRTIO_RNG_PCI(obj);
-object_initialize(dev-vdev, TYPE_VIRTIO_RNG);
+object_initialize(dev-vdev, sizeof(dev-vdev), TYPE_VIRTIO_RNG);
object_property_add_child(obj, virtio-backend, OBJECT(dev-vdev),
NULL);
object_property_add_link(obj, rng, TYPE_RNG_BACKEND,
(Object **)dev-vdev.conf.rng, NULL);
diff --git a/include/qom/object.h b/include/qom/object.h
index c463ced..1a7b71a 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -586,24 +586,26 @@ Object *object_new_with_type(Type type);
/**
* object_initialize_with_type:
* @data: A pointer to the memory to be used for the object.
+ * @size: The maximum size available at @data for the object.
* @type: The type of the object to instantiate.
*
* This function will initialize an object. The memory for the object should
* have already been allocated. The returned object has a reference count of
1,
* and will be finalized when the last reference is dropped.
*/
-void object_initialize_with_type(void *data, Type type);
+void object_initialize_with_type(void *data, size_t size, Type type);
/**
* object_initialize:
* @obj: A pointer to the memory to be used for the object.
+ * @size: The maximum size available at @obj for the object.
* @typename: The name of the type of the object to instantiate.
*
* This function will initialize an object. The memory for the object should
* have already been allocated. The returned object has a reference count of
1,
* and will be finalized when the last reference is dropped.
*/
-void object_initialize(void *obj, const char *typename);
+void object_initialize(void *obj, size_t size, const char *typename);
/**
* object_dynamic_cast:
diff --git a/qom/object.c b/qom/object.c
index 74fd241..e90e382 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -311,7 +311,7 @@ static void object_post_init_with_type(Object *obj,
TypeImpl *ti)
}
}
-void object_initialize_with_type(void *data, TypeImpl *type)
+void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
{
Object *obj = data;
@@ -320,6 +320,7 @@ void object_initialize_with_type(void *data, TypeImpl *type)
g_assert(type-instance_size = sizeof(Object));
g_assert(type-abstract == false);
+g_assert(size = type-instance_size);
I paid some time to find this line for several times, so if it can be
splitted as a separate patch as qom: assert object init size, it will
make review easier.
memset(obj, 0, type-instance_size);
obj-class = type-class;
@@ -329,11 +330,11 @@ void object_initialize_with_type(void *data, TypeImpl
*type)
object_post_init_with_type(obj, type);
}
-void object_initialize(void *data, const char *typename)
+void object_initialize(void *data, size_t size, const char *typename)
{
TypeImpl *type = type_get_by_name(typename);
-object_initialize_with_type(data, type);
+object_initialize_with_type(data, size, type);
}
static inline bool object_property_is_child(ObjectProperty *prop)
@@ -424,7 +425,7 @@ Object *object_new_with_type(Type type)
type_initialize(type);
obj = g_malloc(type-instance_size);
-object_initialize_with_type(obj, type);
+object_initialize_with_type(obj, type-instance_size, type);
obj-free = g_free;
return obj;
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH 00/16] qom: Assert sufficient object instance size
I have not looked deep into QOM, so only reviewed the code in this series, and have some minor comments for patch 12, 13, and 15. For other part, Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com Hello, Peter remarked that object_initialize() on a GICState struct field would not check whether the TypeInfo::instance_size for its typename argument exceeds the length of the field we're initializing. This series therefore updates all callers to explicitly pass the size available for the object. While we don't have that many object_initialize() users yet, quite a few devices are using qbus_create_inplace() or bus-specific functions based on it. Still I consider this the safest solution in pushing the responsability for supplying the length to the caller and thereby checking not only field type lengths but also allocation lengths. The series goes on to showcase an object_initialize() usage where we don't know the instance_size in advance and need a QOM API to obtain it. Based on Peter's OBJECT() elimination patch, which I have queued on qom-next. This series conflicts with my *mpcore, virtio and ipack series among others. Regards, Andreas Cc: Peter Maydell peter.mayd...@linaro.org Cc: Anthony Liguori anth...@codemonkey.ws Andreas Färber (16): qom: Fix object_initialize_with_type() argument name in documentation intel-hda: Pass size to hda_codec_bus_init() ipack: Pass size to ipack_bus_new_inplace() ide: Pass size to ide_bus_new() pci: Pass size to pci_bus_new_inplace() scsi: Pass size to scsi_bus_new() usb: Pass size to usb_bus_new() virtio-pci: Pass size to virtio_pci_bus_new() s390-virtio-bus: Pass size to virtio_s390_bus_new() virtio-ccw: Pass size to virtio_ccw_bus_new() virtio-mmio: Pass size to virtio_mmio_bus_new() qdev: Pass size to qbus_create_inplace() qom: Pass available size to object_initialize() qom: Introduce type_get_instance_size() qdev-monitor: Clean up qdev_device_add() variable naming qdev-monitor: Avoid aborting on out-of-memory in qdev_device_add() hw/audio/intel-hda.c | 6 +++--- hw/audio/intel-hda.h | 2 +- hw/char/ipack.c | 5 +++-- hw/char/ipack.h | 3 ++- hw/char/tpci200.c | 2 +- hw/char/virtio-serial-bus.c | 4 ++-- hw/core/qdev.c| 4 ++-- hw/core/sysbus.c | 4 ++-- hw/cpu/icc_bus.c | 3 ++- hw/dma/xilinx_axidma.c| 6 -- hw/ide/ahci.c | 2 +- hw/ide/cmd646.c | 2 +- hw/ide/internal.h | 3 ++- hw/ide/isa.c | 2 +- hw/ide/macio.c| 2 +- hw/ide/mmio.c | 2 +- hw/ide/piix.c | 2 +- hw/ide/qdev.c | 5 +++-- hw/ide/via.c | 2 +- hw/intc/xics.c| 2 +- hw/misc/macio/cuda.c | 4 ++-- hw/misc/macio/macio.c | 13 +++-- hw/net/xilinx_axienet.c | 6 -- hw/pci-host/prep.c| 4 ++-- hw/pci-host/q35.c | 2 +- hw/pci-host/versatile.c | 4 ++-- hw/pci/pci.c | 4 ++-- hw/pci/pci_bridge.c | 3 ++- hw/s390x/event-facility.c | 4 ++-- hw/s390x/s390-virtio-bus.c| 24 +--- hw/s390x/virtio-ccw.c | 26 ++ hw/scsi/esp-pci.c | 2 +- hw/scsi/esp.c | 2 +- hw/scsi/lsi53c895a.c | 2 +- hw/scsi/megasas.c | 3 ++- hw/scsi/scsi-bus.c| 6 +++--- hw/scsi/spapr_vscsi.c | 3 ++- hw/scsi/virtio-scsi.c | 3 ++- hw/scsi/vmw_pvscsi.c | 3 ++- hw/usb/bus.c | 5 +++-- hw/usb/dev-smartcard-reader.c | 3 ++- hw/usb/dev-storage.c | 6 -- hw/usb/dev-uas.c | 3 ++- hw/usb/hcd-ehci.c | 2 +- hw/usb/hcd-musb.c | 2 +- hw/usb/hcd-ohci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/usb/hcd-xhci.c | 2 +- hw/virtio/virtio-mmio.c | 10 ++ hw/virtio/virtio-pci.c| 26 ++ include/hw/pci/pci.h | 2 +- include/hw/qdev-core.h| 2 +- include/hw/scsi/scsi.h| 4 ++-- include/hw/usb.h | 3 ++- include/qom/object.h | 16 +--- qdev-monitor.c| 30 ++ qom/object.c | 16 57 files changed, 185 insertions(+), 132 deletions(-) -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
于 2013-8-26 23:55, Luiz Capitulino 写道: On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: This series make auto completion and help functions works normal for sub command, by using reentrant functions. In order to do that, global variables are not directly used in those functions any more. With this series, cmd_table is a member of structure Monitor so it is possible to create a monitor with different command table now, auto completion will work in that monitor. In short, info is not treated as a special case now, this series ensure help and auto complete function works normal for any sub command added in the future. Patch 5 replaced cur_mon with rs-mon, it is safe because: monitor_init() calls readline_init() which initialize mon-rs, result is mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make monitor_read() function take *mon as its opaque. Later, when user input, monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque. If qemu's monitors run in one thread, then later in readline_handle_byte() and readline_comletion(), cur_mon is actually equal to rs-mon, in another word, it points to the monitor instance, so it is safe to replace *cur_mon in those functions. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2 Sorry for it, I missed that CC flag in my configure, will fix. It would be great if you can share your configure settings. -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH 0/3] qemu-iotests: add test for fd passing via SCM rights
于 2013-8-26 11:13, Wenchao Xia 写道: This series add test case for fd passing with unix socket at runtime. Since getfd and closefd interface will interact with monitor's data, so it will help to do regression test for monitor patches. Since python2 do not support sendmsg(), so a C helper program is added to do the job. Wenchao Xia (3): 1 qemu-iotests: add unix socket help program 2 qemu-iotests: add infrastructure of fd passing via SCM 3 qemu-iotests: add tests for runtime fd passing via SCM rights QMP/qmp.py |6 ++ configure |2 +- tests/Makefile |4 +- tests/qemu-iotests/045 | 37 ++- tests/qemu-iotests/045.out |4 +- tests/qemu-iotests/check |1 + tests/qemu-iotests/iotests.py | 26 +++ tests/qemu-iotests/socket_scm_helper.c | 119 8 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 tests/qemu-iotests/socket_scm_helper.c This version have one issue in Makefile, will respin. -- Best Regards Wenchao Xia
[Qemu-devel] [PATCH V2 1/3] qemu-iotests: add unix socket help program
This program can do a sendmsg call to transfer fd with unix
socket, which is not supported in python2.
The built binary will not be deleted in clean, but it is a
existing issue in ./tests, which should be solved in another
patch.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
configure |2 +-
tests/Makefile |4 +-
tests/qemu-iotests/socket_scm_helper.c | 119
3 files changed, 123 insertions(+), 2 deletions(-)
create mode 100644 tests/qemu-iotests/socket_scm_helper.c
diff --git a/configure b/configure
index 0a55c20..5080c38 100755
--- a/configure
+++ b/configure
@@ -4544,7 +4544,7 @@ if [ $dtc_internal = yes ]; then
fi
# build tree in object directory in case the source is not in the current
directory
-DIRS=tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos
tests/qapi-schema tests/tcg/xtensa
+DIRS=tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos
tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests
DIRS=$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw
DIRS=$DIRS roms/seabios roms/vgabios
DIRS=$DIRS qapi-generated
diff --git a/tests/Makefile b/tests/Makefile
index baba9e9..d2f3fcb 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -172,6 +172,8 @@ tests/boot-order-test$(EXESUF): tests/boot-order-test.o
$(libqos-obj-y)
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
+tests/qemu-iotests/socket_scm_helper$(EXESUF):
tests/qemu-iotests/socket_scm_helper.o
+ $(call LINK, $^)
# QTest rules
@@ -250,7 +252,7 @@ check-report.html: check-report.xml
# Other tests
.PHONY: check-tests/qemu-iotests-quick.sh
-check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh
qemu-img$(EXESUF) qemu-io$(EXESUF)
+check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh
qemu-img$(EXESUF) qemu-io$(EXESUF) tests/qemu-iotests/socket_scm_helper$(EXESUF)
$
.PHONY: check-tests/test-qapi.py
diff --git a/tests/qemu-iotests/socket_scm_helper.c
b/tests/qemu-iotests/socket_scm_helper.c
new file mode 100644
index 000..1955a3e
--- /dev/null
+++ b/tests/qemu-iotests/socket_scm_helper.c
@@ -0,0 +1,119 @@
+/*
+ * SCM_RIGHT with unix socket help program for test
+ *
+ * Copyright IBM, Inc. 2013
+ *
+ * Authors:
+ * Wenchao Xiaxiaw...@linux.vnet.ibm.com
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include stdio.h
+#include errno.h
+#include sys/socket.h
+#include sys/un.h
+#include stdlib.h
+
+/* #define SOCKET_SCM_DEBUG */
+
+static int send_fd(int fd, int fd_to_send, const char *data, size_t len)
+{
+struct msghdr msg;
+struct iovec iov[1];
+int ret;
+char control[CMSG_SPACE(sizeof(int))];
+struct cmsghdr *cmsg;
+
+if (fd 0) {
+fprintf(stderr, Socket fd is invalid.\n);
+return -1;
+}
+if (fd_to_send 0) {
+fprintf(stderr, Fd to send is invalid.\n);
+return -1;
+}
+if (data == NULL || len = 0) {
+fprintf(stderr, Data buffer must be valid.\n);
+return -1;
+}
+
+memset(msg, 0, sizeof(msg));
+memset(control, 0, sizeof(control));
+
+iov[0].iov_base = (void *)data;
+iov[0].iov_len = len;
+
+msg.msg_iov = iov;
+msg.msg_iovlen = 1;
+
+msg.msg_control = control;
+msg.msg_controllen = sizeof(control);
+
+cmsg = CMSG_FIRSTHDR(msg);
+
+cmsg-cmsg_len = CMSG_LEN(sizeof(int));
+cmsg-cmsg_level = SOL_SOCKET;
+cmsg-cmsg_type = SCM_RIGHTS;
+memcpy(CMSG_DATA(cmsg), fd, sizeof(int));
+
+do {
+ret = sendmsg(fd, msg, 0);
+} while (ret 0 errno == EINTR);
+
+if (ret 0) {
+fprintf(stderr, Failed to send msg, reason: %s.\n, strerror(errno));
+}
+
+return ret;
+}
+
+/*
+ * To make things simple, the caller need to specify:
+ * 1. socket fd.
+ * 2. fd to send.
+ * 3. msg to send.
+ */
+int main(int argc, char **argv, char **envp)
+{
+int sock, fd, ret, buflen;
+const char *buf;
+#ifdef SOCKET_SCM_DEBUG
+int i;
+for (i = 0; i argc; i++) {
+fprintf(stderr, Parameter %d: %s.\n, i, argv[i]);
+}
+#endif
+
+if (argc 4) {
+fprintf(stderr,
+Invalid parameter, use it as:\n
+%s SOCKET_FD FD_TO_SEND MSG.\n,
+argv[0]);
+return 1;
+}
+
+errno = 0;
+sock = strtol(argv[1], NULL, 10);
+if (errno) {
+fprintf(stderr, Failed in strtol for socket fd, reason: %s.\n,
+strerror(errno));
+return 1;
+}
+fd = strtol(argv[2], NULL, 10);
+if (errno) {
+fprintf(stderr, Failed in strtol for fd to send, reason: %s.\n,
+strerror(errno));
+return 1;
+}
+
+buf = argv[3];
+buflen
[Qemu-devel] [PATCH V2 0/3] qemu-iotests: add test for fd passing via SCM rights
This series add test case for fd passing with unix socket at runtime. Since getfd and closefd interface will interact with monitor's data, so it will help to do regression test for monitor patches. Since python2 do not support sendmsg(), so a C helper program is added to do the job. v2: 1: add missing $ in the makefile rule. Wenchao Xia (3): 1 qemu-iotests: add unix socket help program 2 qemu-iotests: add infrastructure of fd passing via SCM 3 qemu-iotests: add tests for runtime fd passing via SCM rights QMP/qmp.py |6 ++ configure |2 +- tests/Makefile |4 +- tests/qemu-iotests/045 | 37 ++- tests/qemu-iotests/045.out |4 +- tests/qemu-iotests/check |1 + tests/qemu-iotests/iotests.py | 26 +++ tests/qemu-iotests/socket_scm_helper.c | 119 8 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 tests/qemu-iotests/socket_scm_helper.c
[Qemu-devel] [PATCH V2 2/3] qemu-iotests: add infrastructure of fd passing via SCM
This patch make use of the compiled scm helper program to transfer
fd via unix socket at runtime.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
QMP/qmp.py|6 ++
tests/qemu-iotests/check |1 +
tests/qemu-iotests/iotests.py | 26 ++
3 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/QMP/qmp.py b/QMP/qmp.py
index c551df1..074f09a 100644
--- a/QMP/qmp.py
+++ b/QMP/qmp.py
@@ -188,3 +188,9 @@ class QEMUMonitorProtocol:
def settimeout(self, timeout):
self.__sock.settimeout(timeout)
+
+def get_sock_fd(self):
+return self.__sock.fileno()
+
+def is_scm_available(self):
+return self.__sock.family == socket.AF_UNIX
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 74628ae..2f7f78e 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -164,6 +164,7 @@ QEMU_IO -- $QEMU_IO
IMGFMT-- $FULL_IMGFMT_DETAILS
IMGPROTO -- $FULL_IMGPROTO_DETAILS
PLATFORM -- $FULL_HOST_DETAILS
+SOCKET_SCM_HELPER -- $SOCKET_SCM_HELPER
EOF
#MKFS_OPTIONS -- $FULL_MKFS_OPTIONS
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 33ad0ec..d40b2de 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -38,6 +38,8 @@ imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
test_dir = os.environ.get('TEST_DIR', '/var/tmp')
+socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
+
def qemu_img(*args):
'''Run qemu-img and return the exit code'''
devnull = open('/dev/null', 'r+')
@@ -80,6 +82,12 @@ class VM(object):
'-display', 'none', '-vga', 'none']
self._num_drives = 0
+#This can be used to add unused monitor
+def add_monitor_telnet(self, ip, port):
+args = 'tcp:%s:%d,server,nowait,telnet' % (ip, port)
+self._args.append('-monitor')
+self._args.append(args)
+
def add_drive(self, path, opts=''):
'''Add a virtio-blk drive to the VM'''
options = ['if=virtio',
@@ -112,6 +120,24 @@ class VM(object):
self._args.append(','.join(options))
return self
+#Sendmsg must carry out some data to get qemu's notice, so send a blank
+#by default.
+def send_fd_scm(self, fd, msg=' '):
+#In iotest.py, the qmp should always use unix socket.
+assert self._qmp.is_scm_available()
+fd_bin = socket_scm_helper
+if os.path.exists(fd_bin) == False:
+print Scm help program does not present, path %s. % fd_bin
+return -1
+fd_param = [%s % fd_bin,
+%d % self._qmp.get_sock_fd(),
+%d % fd,
+%s % msg]
+devnull = open('/dev/null', 'rb')
+p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+ stderr=sys.stderr)
+return p.wait()
+
def launch(self):
'''Launch the VM and establish a QMP connection'''
devnull = open('/dev/null', 'rb')
--
1.7.1
[Qemu-devel] [PATCH V2 3/3] qemu-iotests: add tests for runtime fd passing via SCM rights
This case will test whether the monitor can receive fd at runtime.
To verify better, additional monitor is created to see if qemu
can handler two monitor instance correctly.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
tests/qemu-iotests/045 | 37 -
tests/qemu-iotests/045.out |4 ++--
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/045 b/tests/qemu-iotests/045
index 2b6f1af..f95e5fa 100755
--- a/tests/qemu-iotests/045
+++ b/tests/qemu-iotests/045
@@ -1,6 +1,6 @@
#!/usr/bin/env python
#
-# Tests for fdsets.
+# Tests for fdsets and getfd.
#
# Copyright (C) 2012 IBM Corp.
#
@@ -125,5 +125,40 @@ class TestFdSets(iotests.QMPTestCase):
'No file descriptor supplied via SCM_RIGHTS')
self.vm.shutdown()
+#Add fd at runtime, there are two ways: monitor related or fdset related
+class TestSCMFd(iotests.QMPTestCase):
+def setUp(self):
+self.vm = iotests.VM()
+qemu_img('create', '-f', iotests.imgfmt, image0, '128K')
+self.file0 = open(image0, 'r')
+self.vm.add_monitor_telnet(0,4445)
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+self.file0.close()
+os.remove(image0)
+
+def _send_fd_by_SCM(self):
+ret = self.vm.send_fd_scm(self.file0.fileno())
+self.assertEqual(ret, 0, 'Failed to send fd with UNIX SCM')
+
+def test_add_fd(self):
+self._send_fd_by_SCM()
+result = self.vm.qmp('add-fd', fdset_id=2, opaque='image0:r')
+self.assert_qmp(result, 'return/fdset-id', 2)
+
+def test_getfd(self):
+self._send_fd_by_SCM()
+result = self.vm.qmp('getfd', fdname='image0:r')
+self.assert_qmp(result, 'return', {})
+
+def test_closefd(self):
+self._send_fd_by_SCM()
+result = self.vm.qmp('getfd', fdname='image0:r')
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp('closefd', fdname='image0:r')
+self.assert_qmp(result, 'return', {})
+
if __name__ == '__main__':
iotests.main(supported_fmts=['raw'])
diff --git a/tests/qemu-iotests/045.out b/tests/qemu-iotests/045.out
index 3f8a935..dae404e 100644
--- a/tests/qemu-iotests/045.out
+++ b/tests/qemu-iotests/045.out
@@ -1,5 +1,5 @@
-..
+.
--
-Ran 6 tests
+Ran 9 tests
OK
--
1.7.1
[Qemu-devel] [PATCH 0/3] qemu-iotests: add test for fd passing via SCM rights
This series add test case for fd passing with unix socket at runtime. Since getfd and closefd interface will interact with monitor's data, so it will help to do regression test for monitor patches. Since python2 do not support sendmsg(), so a C helper program is added to do the job. Wenchao Xia (3): 1 qemu-iotests: add unix socket help program 2 qemu-iotests: add infrastructure of fd passing via SCM 3 qemu-iotests: add tests for runtime fd passing via SCM rights QMP/qmp.py |6 ++ configure |2 +- tests/Makefile |4 +- tests/qemu-iotests/045 | 37 ++- tests/qemu-iotests/045.out |4 +- tests/qemu-iotests/check |1 + tests/qemu-iotests/iotests.py | 26 +++ tests/qemu-iotests/socket_scm_helper.c | 119 8 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 tests/qemu-iotests/socket_scm_helper.c
[Qemu-devel] [PATCH 1/3] qemu-iotests: add unix socket help program
This program can do a sendmsg call to transfer fd with unix
socket, which is not supported in python2.
The built binary will not be deleted in clean, but it is a
existing issue in ./tests, which should be solved in another
patch.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
configure |2 +-
tests/Makefile |4 +-
tests/qemu-iotests/socket_scm_helper.c | 119
3 files changed, 123 insertions(+), 2 deletions(-)
create mode 100644 tests/qemu-iotests/socket_scm_helper.c
diff --git a/configure b/configure
index 18fa608..0a18038 100755
--- a/configure
+++ b/configure
@@ -4507,7 +4507,7 @@ if [ $dtc_internal = yes ]; then
fi
# build tree in object directory in case the source is not in the current
directory
-DIRS=tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos
tests/qapi-schema tests/tcg/xtensa
+DIRS=tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos
tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests
DIRS=$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw
DIRS=$DIRS roms/seabios roms/vgabios
DIRS=$DIRS qapi-generated
diff --git a/tests/Makefile b/tests/Makefile
index baba9e9..57131db 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -172,6 +172,8 @@ tests/boot-order-test$(EXESUF): tests/boot-order-test.o
$(libqos-obj-y)
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
+tests/qemu-iotests/socket_scm_helper(EXESUF):
tests/qemu-iotests/socket_scm_helper.o
+ $(call LINK, $^)
# QTest rules
@@ -250,7 +252,7 @@ check-report.html: check-report.xml
# Other tests
.PHONY: check-tests/qemu-iotests-quick.sh
-check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh
qemu-img$(EXESUF) qemu-io$(EXESUF)
+check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh
qemu-img$(EXESUF) qemu-io$(EXESUF) tests/qemu-iotests/socket_scm_helper$(EXESUF)
$
.PHONY: check-tests/test-qapi.py
diff --git a/tests/qemu-iotests/socket_scm_helper.c
b/tests/qemu-iotests/socket_scm_helper.c
new file mode 100644
index 000..1955a3e
--- /dev/null
+++ b/tests/qemu-iotests/socket_scm_helper.c
@@ -0,0 +1,119 @@
+/*
+ * SCM_RIGHT with unix socket help program for test
+ *
+ * Copyright IBM, Inc. 2013
+ *
+ * Authors:
+ * Wenchao Xiaxiaw...@linux.vnet.ibm.com
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include stdio.h
+#include errno.h
+#include sys/socket.h
+#include sys/un.h
+#include stdlib.h
+
+/* #define SOCKET_SCM_DEBUG */
+
+static int send_fd(int fd, int fd_to_send, const char *data, size_t len)
+{
+struct msghdr msg;
+struct iovec iov[1];
+int ret;
+char control[CMSG_SPACE(sizeof(int))];
+struct cmsghdr *cmsg;
+
+if (fd 0) {
+fprintf(stderr, Socket fd is invalid.\n);
+return -1;
+}
+if (fd_to_send 0) {
+fprintf(stderr, Fd to send is invalid.\n);
+return -1;
+}
+if (data == NULL || len = 0) {
+fprintf(stderr, Data buffer must be valid.\n);
+return -1;
+}
+
+memset(msg, 0, sizeof(msg));
+memset(control, 0, sizeof(control));
+
+iov[0].iov_base = (void *)data;
+iov[0].iov_len = len;
+
+msg.msg_iov = iov;
+msg.msg_iovlen = 1;
+
+msg.msg_control = control;
+msg.msg_controllen = sizeof(control);
+
+cmsg = CMSG_FIRSTHDR(msg);
+
+cmsg-cmsg_len = CMSG_LEN(sizeof(int));
+cmsg-cmsg_level = SOL_SOCKET;
+cmsg-cmsg_type = SCM_RIGHTS;
+memcpy(CMSG_DATA(cmsg), fd, sizeof(int));
+
+do {
+ret = sendmsg(fd, msg, 0);
+} while (ret 0 errno == EINTR);
+
+if (ret 0) {
+fprintf(stderr, Failed to send msg, reason: %s.\n, strerror(errno));
+}
+
+return ret;
+}
+
+/*
+ * To make things simple, the caller need to specify:
+ * 1. socket fd.
+ * 2. fd to send.
+ * 3. msg to send.
+ */
+int main(int argc, char **argv, char **envp)
+{
+int sock, fd, ret, buflen;
+const char *buf;
+#ifdef SOCKET_SCM_DEBUG
+int i;
+for (i = 0; i argc; i++) {
+fprintf(stderr, Parameter %d: %s.\n, i, argv[i]);
+}
+#endif
+
+if (argc 4) {
+fprintf(stderr,
+Invalid parameter, use it as:\n
+%s SOCKET_FD FD_TO_SEND MSG.\n,
+argv[0]);
+return 1;
+}
+
+errno = 0;
+sock = strtol(argv[1], NULL, 10);
+if (errno) {
+fprintf(stderr, Failed in strtol for socket fd, reason: %s.\n,
+strerror(errno));
+return 1;
+}
+fd = strtol(argv[2], NULL, 10);
+if (errno) {
+fprintf(stderr, Failed in strtol for fd to send, reason: %s.\n,
+strerror(errno));
+return 1;
+}
+
+buf = argv[3];
+buflen
[Qemu-devel] [PATCH 2/3] qemu-iotests: add infrastructure of fd passing via SCM
This patch make use of the compiled scm helper program to transfer
fd via unix socket at runtime.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
QMP/qmp.py|6 ++
tests/qemu-iotests/check |1 +
tests/qemu-iotests/iotests.py | 26 ++
3 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/QMP/qmp.py b/QMP/qmp.py
index c551df1..074f09a 100644
--- a/QMP/qmp.py
+++ b/QMP/qmp.py
@@ -188,3 +188,9 @@ class QEMUMonitorProtocol:
def settimeout(self, timeout):
self.__sock.settimeout(timeout)
+
+def get_sock_fd(self):
+return self.__sock.fileno()
+
+def is_scm_available(self):
+return self.__sock.family == socket.AF_UNIX
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 74628ae..2f7f78e 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -164,6 +164,7 @@ QEMU_IO -- $QEMU_IO
IMGFMT-- $FULL_IMGFMT_DETAILS
IMGPROTO -- $FULL_IMGPROTO_DETAILS
PLATFORM -- $FULL_HOST_DETAILS
+SOCKET_SCM_HELPER -- $SOCKET_SCM_HELPER
EOF
#MKFS_OPTIONS -- $FULL_MKFS_OPTIONS
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 33ad0ec..d40b2de 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -38,6 +38,8 @@ imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
test_dir = os.environ.get('TEST_DIR', '/var/tmp')
+socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
+
def qemu_img(*args):
'''Run qemu-img and return the exit code'''
devnull = open('/dev/null', 'r+')
@@ -80,6 +82,12 @@ class VM(object):
'-display', 'none', '-vga', 'none']
self._num_drives = 0
+#This can be used to add unused monitor
+def add_monitor_telnet(self, ip, port):
+args = 'tcp:%s:%d,server,nowait,telnet' % (ip, port)
+self._args.append('-monitor')
+self._args.append(args)
+
def add_drive(self, path, opts=''):
'''Add a virtio-blk drive to the VM'''
options = ['if=virtio',
@@ -112,6 +120,24 @@ class VM(object):
self._args.append(','.join(options))
return self
+#Sendmsg must carry out some data to get qemu's notice, so send a blank
+#by default.
+def send_fd_scm(self, fd, msg=' '):
+#In iotest.py, the qmp should always use unix socket.
+assert self._qmp.is_scm_available()
+fd_bin = socket_scm_helper
+if os.path.exists(fd_bin) == False:
+print Scm help program does not present, path %s. % fd_bin
+return -1
+fd_param = [%s % fd_bin,
+%d % self._qmp.get_sock_fd(),
+%d % fd,
+%s % msg]
+devnull = open('/dev/null', 'rb')
+p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+ stderr=sys.stderr)
+return p.wait()
+
def launch(self):
'''Launch the VM and establish a QMP connection'''
devnull = open('/dev/null', 'rb')
--
1.7.1
[Qemu-devel] [PATCH 3/3] qemu-iotests: add tests for runtime fd passing via SCM rights
This case will test whether the monitor can receive fd at runtime.
To verify better, additional monitor is created to see if qemu
can handler two monitor instance correctly.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
tests/qemu-iotests/045 | 37 -
tests/qemu-iotests/045.out |4 ++--
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/045 b/tests/qemu-iotests/045
index 2b6f1af..f95e5fa 100755
--- a/tests/qemu-iotests/045
+++ b/tests/qemu-iotests/045
@@ -1,6 +1,6 @@
#!/usr/bin/env python
#
-# Tests for fdsets.
+# Tests for fdsets and getfd.
#
# Copyright (C) 2012 IBM Corp.
#
@@ -125,5 +125,40 @@ class TestFdSets(iotests.QMPTestCase):
'No file descriptor supplied via SCM_RIGHTS')
self.vm.shutdown()
+#Add fd at runtime, there are two ways: monitor related or fdset related
+class TestSCMFd(iotests.QMPTestCase):
+def setUp(self):
+self.vm = iotests.VM()
+qemu_img('create', '-f', iotests.imgfmt, image0, '128K')
+self.file0 = open(image0, 'r')
+self.vm.add_monitor_telnet(0,4445)
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+self.file0.close()
+os.remove(image0)
+
+def _send_fd_by_SCM(self):
+ret = self.vm.send_fd_scm(self.file0.fileno())
+self.assertEqual(ret, 0, 'Failed to send fd with UNIX SCM')
+
+def test_add_fd(self):
+self._send_fd_by_SCM()
+result = self.vm.qmp('add-fd', fdset_id=2, opaque='image0:r')
+self.assert_qmp(result, 'return/fdset-id', 2)
+
+def test_getfd(self):
+self._send_fd_by_SCM()
+result = self.vm.qmp('getfd', fdname='image0:r')
+self.assert_qmp(result, 'return', {})
+
+def test_closefd(self):
+self._send_fd_by_SCM()
+result = self.vm.qmp('getfd', fdname='image0:r')
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp('closefd', fdname='image0:r')
+self.assert_qmp(result, 'return', {})
+
if __name__ == '__main__':
iotests.main(supported_fmts=['raw'])
diff --git a/tests/qemu-iotests/045.out b/tests/qemu-iotests/045.out
index 3f8a935..dae404e 100644
--- a/tests/qemu-iotests/045.out
+++ b/tests/qemu-iotests/045.out
@@ -1,5 +1,5 @@
-..
+.
--
-Ran 6 tests
+Ran 9 tests
OK
--
1.7.1
[Qemu-devel] [PATCH V9 08/15] monitor: avoid direct use of global variable *mon_cmds
New member *cmd_table is added in structure Monitor to avoid direct usage of
*mon_cmds. Now monitor have an associated command table, when global variable
*info_cmds is also discarded, structure Monitor would gain full control about
how to deal with user input.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
monitor.c | 13 -
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/monitor.c b/monitor.c
index 3ac98ef..8152607 100644
--- a/monitor.c
+++ b/monitor.c
@@ -195,6 +195,7 @@ struct Monitor {
CPUState *mon_cpu;
BlockDriverCompletionFunc *password_completion_cb;
void *password_opaque;
+mon_cmd_t *cmd_table;
QError *error;
QLIST_HEAD(,mon_fd_t) fds;
QLIST_ENTRY(Monitor) entry;
@@ -687,6 +688,8 @@ static void monitor_data_init(Monitor *mon)
{
memset(mon, 0, sizeof(Monitor));
mon-outbuf = qstring_new();
+/* Use *mon_cmds by default. */
+mon-cmd_table = mon_cmds;
}
static void monitor_data_destroy(Monitor *mon)
@@ -767,7 +770,7 @@ static void help_cmd(Monitor *mon, const char *name)
if (name !strcmp(name, info)) {
help_cmd_dump(mon, info_cmds, info , NULL);
} else {
-help_cmd_dump(mon, mon_cmds, , name);
+help_cmd_dump(mon, mon-cmd_table, , name);
if (name !strcmp(name, log)) {
const QEMULogItem *item;
monitor_printf(mon, Log items (comma separated):\n);
@@ -3994,7 +3997,7 @@ static void handle_user_command(Monitor *mon, const char
*cmdline)
qdict = qdict_new();
-cmd = monitor_parse_command(mon, cmdline, 0, mon_cmds, qdict);
+cmd = monitor_parse_command(mon, cmdline, 0, mon-cmd_table, qdict);
if (!cmd)
goto out;
@@ -4183,12 +4186,12 @@ static void monitor_find_completion(Monitor *mon,
else
cmdname = args[0];
readline_set_completion_index(mon-rs, strlen(cmdname));
-for(cmd = mon_cmds; cmd-name != NULL; cmd++) {
+for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
cmd_completion(mon, cmdname, cmd-name);
}
} else {
/* find the command */
-for (cmd = mon_cmds; cmd-name != NULL; cmd++) {
+for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
if (compare_cmd(args[0], cmd-name)) {
break;
}
@@ -4239,7 +4242,7 @@ static void monitor_find_completion(Monitor *mon,
}
} else if (!strcmp(cmd-name, help|?)) {
readline_set_completion_index(mon-rs, strlen(str));
-for (cmd = mon_cmds; cmd-name != NULL; cmd++) {
+for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
cmd_completion(mon, str, cmd-name);
}
}
--
1.7.1
[Qemu-devel] [PATCH V9 03/15] monitor: avoid use of global *cur_mon in block_completion_it()
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 18 ++
1 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/monitor.c b/monitor.c
index 30819fa..6171c75 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4090,14 +4090,21 @@ static void file_completion(Monitor *mon, const char
*input)
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);
}
}
@@ -4141,6 +4148,7 @@ static void monitor_find_completion(const char *cmdline)
const char *ptype, *str;
const mon_cmd_t *cmd;
Monitor *mon = cur_mon;
+MonitorBlockComplete mbs;
parse_cmdline(cmdline, nb_args, args);
#ifdef DEBUG_COMPLETION
@@ -4199,8 +4207,10 @@ static void monitor_find_completion(const char *cmdline)
break;
case 'B':
/* block device name completion */
-readline_set_completion_index(cur_mon-rs, strlen(str));
-bdrv_iterate(block_completion_it, (void *)str);
+mbs.mon = mon;
+mbs.input = str;
+readline_set_completion_index(mon-rs, strlen(str));
+bdrv_iterate(block_completion_it, mbs);
break;
case 's':
/* XXX: more generic ? */
--
1.7.1
[Qemu-devel] [PATCH V9 01/15] monitor: avoid use of global *cur_mon in cmd_completion()
A new local variable *mon is added in monitor_find_completion()
to make compile pass, which will be removed later in
conversion patch for monitor_find_completion().
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 13 +++--
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/monitor.c b/monitor.c
index da9c9a2..e0154a8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4008,7 +4008,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];
@@ -4026,7 +4026,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;
@@ -4140,6 +4140,7 @@ static void monitor_find_completion(const char *cmdline)
int nb_args, i, len;
const char *ptype, *str;
const mon_cmd_t *cmd;
+Monitor *mon = cur_mon;
parse_cmdline(cmdline, nb_args, args);
#ifdef DEBUG_COMPLETION
@@ -4165,7 +4166,7 @@ static void monitor_find_completion(const char *cmdline)
cmdname = args[0];
readline_set_completion_index(cur_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 */
@@ -4206,7 +4207,7 @@ static void monitor_find_completion(const char *cmdline)
if (!strcmp(cmd-name, info)) {
readline_set_completion_index(cur_mon-rs, strlen(str));
for(cmd = info_cmds; cmd-name != NULL; cmd++) {
-cmd_completion(str, cmd-name);
+cmd_completion(mon, str, cmd-name);
}
} else if (!strcmp(cmd-name, sendkey)) {
char *sep = strrchr(str, '-');
@@ -4214,12 +4215,12 @@ static void monitor_find_completion(const char *cmdline)
str = sep + 1;
readline_set_completion_index(cur_mon-rs, strlen(str));
for (i = 0; i Q_KEY_CODE_MAX; i++) {
-cmd_completion(str, QKeyCode_lookup[i]);
+cmd_completion(mon, str, QKeyCode_lookup[i]);
}
} else if (!strcmp(cmd-name, help|?)) {
readline_set_completion_index(cur_mon-rs, strlen(str));
for (cmd = mon_cmds; cmd-name != NULL; cmd++) {
-cmd_completion(str, cmd-name);
+cmd_completion(mon, str, cmd-name);
}
}
break;
--
1.7.1
[Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
This series make auto completion and help functions works normal for sub command, by using reentrant functions. In order to do that, global variables are not directly used in those functions any more. With this series, cmd_table is a member of structure Monitor so it is possible to create a monitor with different command table now, auto completion will work in that monitor. In short, info is not treated as a special case now, this series ensure help and auto complete function works normal for any sub command added in the future. Patch 5 replaced cur_mon with rs-mon, it is safe because: monitor_init() calls readline_init() which initialize mon-rs, result is mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make monitor_read() function take *mon as its opaque. Later, when user input, monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque. If qemu's monitors run in one thread, then later in readline_handle_byte() and readline_comletion(), cur_mon is actually equal to rs-mon, in another word, it points to the monitor instance, so it is safe to replace *cur_mon in those functions. Thanks for Luiz and Eric for reviewing. V2: General: To discard *info_comds more graceful, help related function is modified to support sub command too. Patch 6/7 are added to improve help related functions. Patch 5: not directly return to make sure args are freed. Address Luiz's comments: Split patch into small series. struct mon_cmd_t was not moved into header file, instead mon_cmnd_t *cmd_table is added as a member in struct Monitor. 5/7: drop original code comments for info in monitor_find_completion(). v3: 5/7: add parameter **args_cmdline in parse_cmdline() to tell next valid parameter's position. This fix the issue in case command length in input is not equal to its name's length such as help|?, and the case input start with space such as s. 7/7: better commit message. v4: Address Eric's comments: 1/7, 2/7, 4/7: better commit title and message. 1/7 remove useless (char *) in old code, add space around for () in old code. 3/7: separate code moving patch before usage. 4/7: add space around for () in old code, add min(nb_args, MAX_ARGS) in free to make code stronger. v5: 4/7: use a b ? a : b instead of macro min. v6: Address Luiz's comments: 1/13 ~ 5/13: splitted small patches. 5/13: added commit message about the correctness of replacing of cur_mon and test result. 6/13: better comments in code. 7/13: added commit message about the reason of code moving. 8/13: new patch to improve parse_cmdline(), since it is a more generic function now. 9/13: reworked the commit message, better commentes in code, use free_cmdline_args() in clean. It is a bit hard to split this patch into smaller meaning ful ones, so kepted this patch as a relative larger one, with better commit message. 12/13: put case 'S' with case 's' in monitor_find_completion_by_table(). moved this patch ahead of patch 13/13. 13/13: this patch is moved behind patch 12/13. Generic change: 10/13: splitted patch which moved out the reentrant part into a separate function, make review easier. This also avoided re-parsing the command line which does in previous version. 11/13: splitted patch, which simply remove usage of info_cmds and support sub command by re-enter the function. v7: Address Luiz's comments: 5/13: moved the comments why the change is safe, to cover-letter. 8/13: use assert in free_cmdline_args(), fail when args in input exceed the limit in parse_cmdline(). v8: Address Eric's comments: Fix typo in commit messages. V9: General: 6: new patch, I found that call can be optimized, so move it. Address Luiz's comments: 7: split function to init the monitor. 8: move the cmd_table init code to parse_cmdlinethe splitted function. 11: directly return in fail calling parse_cmdline(), in help_cmd_dump(). Other: After discuss with Luiz, it would be good to add test case for qmp fd-add related stuff. Current test case is 045, which doesn't cover the case that fd is added by SCM at runtime, and that case is related to monitor. I have drafted a version, which can pass both on upstream and this series, but it may require a bit more code, so send it later as another series. Wenchao Xia (15): 1 monitor: avoid use of global *cur_mon in cmd_completion() 2 monitor: avoid use of global *cur_mon in file_completion() 3 monitor: avoid use of global *cur_mon in block_completion_it() 4 monitor: avoid use of global *cur_mon in monitor_find_completion() 5 monitor: avoid use of global *cur_mon in readline_completion() 6 monitor: call sortcmdlist() only one time 7 monitor: split off monitor_data_init() 8 monitor: avoid direct use of global variable *mon_cmds 9 monitor: code move for parse_cmdline() 10 monitor: refine parse_cmdline() 11 monitor: support sub command in help 12 monitor: refine monitor_find_completion() 13 monitor: support
[Qemu-devel] [PATCH V9 04/15] monitor: avoid use of global *cur_mon in monitor_find_completion()
Parameter *mon is added, and local variable *mon added in previous patch
is removed. The caller readline_completion(), pass rs-mon as value, which
should be initialized in readline_init() called by monitor_init().
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
include/monitor/readline.h |3 ++-
monitor.c | 18 +-
readline.c |2 +-
3 files changed, 12 insertions(+), 11 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 6171c75..63a779f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4140,20 +4140,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;
-Monitor *mon = cur_mon;
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
@@ -4172,7 +4172,7 @@ 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(mon, cmdname, cmd-name);
}
@@ -4202,7 +4202,7 @@ static void monitor_find_completion(const char *cmdline)
switch(*ptype) {
case 'F':
/* file completion */
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
file_completion(mon, str);
break;
case 'B':
@@ -4215,7 +4215,7 @@ static void monitor_find_completion(const char *cmdline)
case 's':
/* XXX: more generic ? */
if (!strcmp(cmd-name, info)) {
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
for(cmd = info_cmds; cmd-name != NULL; cmd++) {
cmd_completion(mon, str, cmd-name);
}
@@ -4223,12 +4223,12 @@ static void monitor_find_completion(const char *cmdline)
char *sep = strrchr(str, '-');
if (sep)
str = sep + 1;
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
for (i = 0; i Q_KEY_CODE_MAX; i++) {
cmd_completion(mon, str, QKeyCode_lookup[i]);
}
} else if (!strcmp(cmd-name, help|?)) {
-readline_set_completion_index(cur_mon-rs, strlen(str));
+readline_set_completion_index(mon-rs, strlen(str));
for (cmd = mon_cmds; cmd-name != NULL; cmd++) {
cmd_completion(mon, str, cmd-name);
}
diff --git a/readline.c b/readline.c
index 1c0f7ee..c91b324 100644
--- a/readline.c
+++ b/readline.c
@@ -285,7 +285,7 @@ static void readline_completion(ReadLineState *rs)
cmdline = g_malloc(rs-cmd_buf_index + 1);
memcpy(cmdline, rs-cmd_buf, rs-cmd_buf_index);
cmdline[rs-cmd_buf_index] = '\0';
-rs-completion_finder(cmdline);
+rs-completion_finder(rs-mon, cmdline);
g_free(cmdline);
/* no completion found */
--
1.7.1
[Qemu-devel] [PATCH V9 06/15] monitor: call sortcmdlist() only one time
It doesn't need to be done for every monitor, so change it.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
monitor.c |3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/monitor.c b/monitor.c
index 63a779f..bf019e2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4762,6 +4762,7 @@ void monitor_init(CharDriverState *chr, int flags)
if (is_first_init) {
monitor_protocol_event_init();
+sortcmdlist();
is_first_init = 0;
}
@@ -4791,8 +4792,6 @@ void monitor_init(CharDriverState *chr, int flags)
QLIST_INSERT_HEAD(mon_list, mon, entry);
if (!default_mon || (flags MONITOR_IS_DEFAULT))
default_mon = mon;
-
-sortcmdlist();
}
static void bdrv_password_cb(Monitor *mon, const char *password, void *opaque)
--
1.7.1
[Qemu-devel] [PATCH V9 10/15] monitor: refine parse_cmdline()
Since this function will be used by help_cmd() later, so improve
it to make it more generic and easier to use. free_cmdline_args()
is added too as paired function to free the result.
One change of this function is that, when the valid args in input
exceed the limit of MAX_ARGS, it fails now, instead of return with
MAX_ARGS of parsed args in old code. This should not impact much
since it is rare that user input many args in monitor's help and
auto complete scenario.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 51 ---
1 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/monitor.c b/monitor.c
index 1db0117..8897291 100644
--- a/monitor.c
+++ b/monitor.c
@@ -821,9 +821,33 @@ static int get_str(char *buf, int buf_size, const char
**pp)
#define MAX_ARGS 16
-/* NOTE: this parser is an approximate form of the real command parser */
-static void parse_cmdline(const char *cmdline,
- int *pnb_args, char **args)
+static void free_cmdline_args(char **args, int nb_args)
+{
+int i;
+
+assert(nb_args = MAX_ARGS);
+
+for (i = 0; i nb_args; i++) {
+g_free(args[i]);
+}
+
+}
+
+/*
+ * Parse the command line to get valid args.
+ * @cmdline: command line to be parsed.
+ * @pnb_args: location to store the number of args, must NOT be NULL.
+ * @args: location to store the args, which should be freed by caller, must
+ *NOT be NULL.
+ *
+ * Returns 0 on success, negative on failure.
+ *
+ * NOTE: this parser is an approximate form of the real command parser. Number
+ * of args have a limit of MAX_ARGS. If cmdline contains more, it will
+ * return with failure.
+ */
+static int parse_cmdline(const char *cmdline,
+ int *pnb_args, char **args)
{
const char *p;
int nb_args, ret;
@@ -839,16 +863,21 @@ static void parse_cmdline(const char *cmdline,
break;
}
if (nb_args = MAX_ARGS) {
-break;
+goto fail;
}
ret = get_str(buf, sizeof(buf), p);
-args[nb_args] = g_strdup(buf);
-nb_args++;
if (ret 0) {
-break;
+goto fail;
}
+args[nb_args] = g_strdup(buf);
+nb_args++;
}
*pnb_args = nb_args;
+return 0;
+
+ fail:
+free_cmdline_args(args, nb_args);
+return -1;
}
static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
@@ -4168,7 +4197,9 @@ static void monitor_find_completion(Monitor *mon,
const mon_cmd_t *cmd;
MonitorBlockComplete mbs;
-parse_cmdline(cmdline, nb_args, args);
+if (parse_cmdline(cmdline, nb_args, args) 0) {
+return;
+}
#ifdef DEBUG_COMPLETION
for (i = 0; i nb_args; i++) {
monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
@@ -4258,9 +4289,7 @@ static void monitor_find_completion(Monitor *mon,
}
cleanup:
-for (i = 0; i nb_args; i++) {
-g_free(args[i]);
-}
+free_cmdline_args(args, nb_args);
}
static int monitor_can_read(void *opaque)
--
1.7.1
[Qemu-devel] [PATCH V9 11/15] monitor: support sub command in help
The old code in help_cmd() uses global 'info_cmds' and treats it as a
special case. Actually 'info_cmds' is a sub command group of 'mon_cmds',
in order to avoid direct use of it, help_cmd() needs to change its work
mechanism to support sub command and not treat it as a special case
any more.
To support sub command, help_cmd() will first parse the input and then call
help_cmd_dump(), which works as a reentrant function. When it meets a sub
command, it simply enters the function again. Since help dumping needs to
know whole input to printf full help message include prefix, for example,
help info block need to printf prefix info, so help_cmd_dump() takes all
args from input and extra parameter arg_index to identify the progress.
Another function help_cmd_dump_one() is introduced to printf the prefix
and command's help message.
Now help supports sub command, so later if another sub command group is
added in any depth, help will automatically work for it. Still help info
block will show error since command parser reject additional parameter,
which can be improved later. log is still treated as a special case.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 63 +++-
1 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/monitor.c b/monitor.c
index 8897291..1ace29e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -880,33 +880,76 @@ static int parse_cmdline(const char *cmdline,
return -1;
}
+static void help_cmd_dump_one(Monitor *mon,
+ const mon_cmd_t *cmd,
+ char **prefix_args,
+ int prefix_args_nb)
+{
+int i;
+
+for (i = 0; i prefix_args_nb; i++) {
+monitor_printf(mon, %s , prefix_args[i]);
+}
+monitor_printf(mon, %s %s -- %s\n, cmd-name, cmd-params, cmd-help);
+}
+
+/* @args[@arg_index] is the valid command need to find in @cmds */
static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
- const char *prefix, const char *name)
+ char **args, int nb_args, int arg_index)
{
const mon_cmd_t *cmd;
-for(cmd = cmds; cmd-name != NULL; cmd++) {
-if (!name || !strcmp(name, cmd-name))
-monitor_printf(mon, %s%s %s -- %s\n, prefix, cmd-name,
- cmd-params, cmd-help);
+/* No valid arg need to compare with, dump all in *cmds */
+if (arg_index = nb_args) {
+for (cmd = cmds; cmd-name != NULL; cmd++) {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+return;
+}
+
+/* Find one entry to dump */
+for (cmd = cmds; cmd-name != NULL; cmd++) {
+if (compare_cmd(args[arg_index], cmd-name)) {
+if (cmd-sub_table) {
+/* continue with next arg */
+help_cmd_dump(mon, cmd-sub_table,
+ args, nb_args, arg_index + 1);
+} else {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+break;
+}
}
}
static void help_cmd(Monitor *mon, const char *name)
{
-if (name !strcmp(name, info)) {
-help_cmd_dump(mon, info_cmds, info , NULL);
-} else {
-help_cmd_dump(mon, mon-cmd_table, , name);
-if (name !strcmp(name, log)) {
+char *args[MAX_ARGS];
+int nb_args = 0;
+
+/* 1. parse user input */
+if (name) {
+/* special case for log, directly dump and return */
+if (!strcmp(name, log)) {
const QEMULogItem *item;
monitor_printf(mon, Log items (comma separated):\n);
monitor_printf(mon, %-10s %s\n, none, remove all logs);
for (item = qemu_log_items; item-mask != 0; item++) {
monitor_printf(mon, %-10s %s\n, item-name, item-help);
}
+return;
+}
+
+if (parse_cmdline(name, nb_args, args) 0) {
+return;
}
}
+
+/* 2. dump the contents according to parsed args */
+help_cmd_dump(mon, mon-cmd_table, args, nb_args, 0);
+
+cleanup:
+free_cmdline_args(args, nb_args);
}
static void do_help_cmd(Monitor *mon, const QDict *qdict)
--
1.7.1
[Qemu-devel] [PATCH V9 05/15] monitor: avoid use of global *cur_mon in readline_completion()
Now all completion functions do not use *cur_mon any more, instead
they use rs-mon. In short, structure ReadLineState decide where
the complete action would be taken now.
Tested with the case that qemu have two telnet monitors, auto
completion function works normal.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
readline.c |3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/readline.c b/readline.c
index c91b324..abf27dd 100644
--- a/readline.c
+++ b/readline.c
@@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int
index)
static void readline_completion(ReadLineState *rs)
{
-Monitor *mon = cur_mon;
int len, i, j, max_width, nb_cols, max_prefix;
char *cmdline;
@@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs)
if (len 0 rs-completions[0][len - 1] != '/')
readline_insert_char(rs, ' ');
} else {
-monitor_printf(mon, \n);
+monitor_printf(rs-mon, \n);
max_width = 0;
max_prefix = 0;
for(i = 0; i rs-nb_completions; i++) {
--
1.7.1
[Qemu-devel] [PATCH V9 02/15] monitor: avoid use of global *cur_mon in file_completion()
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/monitor.c b/monitor.c
index e0154a8..30819fa 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4034,7 +4034,7 @@ static void cmd_completion(Monitor *mon, 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;
@@ -4057,7 +4057,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);
@@ -4084,7 +4084,7 @@ 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);
@@ -4195,7 +4195,7 @@ static void monitor_find_completion(const char *cmdline)
case 'F':
/* file completion */
readline_set_completion_index(cur_mon-rs, strlen(str));
-file_completion(str);
+file_completion(mon, str);
break;
case 'B':
/* block device name completion */
--
1.7.1
[Qemu-devel] [PATCH V9 12/15] monitor: refine monitor_find_completion()
In order to support sub command in auto completion, a reentrant function
is needed, so monitor_find_completion() is split into two parts. The
first part does parsing of user input which need to be done only once,
the second part does the auto completion job according to the parsing
result, which contains the necessary code to support sub command and
works as the reentrant function. The global info_cmds is still used
in second part, which will be replaced by sub command code later.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 65
1 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/monitor.c b/monitor.c
index 1ace29e..49a5a88 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4230,34 +4230,17 @@ static const char *next_arg_type(const char *typestr)
return (p != NULL ? ++p : typestr);
}
-static void monitor_find_completion(Monitor *mon,
-const char *cmdline)
+static void monitor_find_completion_by_table(Monitor *mon,
+ const mon_cmd_t *cmd_table,
+ char **args,
+ int nb_args)
{
const char *cmdname;
-char *args[MAX_ARGS];
-int nb_args, i, len;
+int i;
const char *ptype, *str;
const mon_cmd_t *cmd;
MonitorBlockComplete mbs;
-if (parse_cmdline(cmdline, nb_args, args) 0) {
-return;
-}
-#ifdef DEBUG_COMPLETION
-for (i = 0; i nb_args; i++) {
-monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
-}
-#endif
-
-/* if the line ends with a space, it means we want to complete the
- next arg */
-len = strlen(cmdline);
-if (len 0 qemu_isspace(cmdline[len - 1])) {
-if (nb_args = MAX_ARGS) {
-goto cleanup;
-}
-args[nb_args++] = g_strdup();
-}
if (nb_args = 1) {
/* command completion */
if (nb_args == 0)
@@ -4265,18 +4248,18 @@ static void monitor_find_completion(Monitor *mon,
else
cmdname = args[0];
readline_set_completion_index(mon-rs, strlen(cmdname));
-for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
+for (cmd = cmd_table; cmd-name != NULL; cmd++) {
cmd_completion(mon, cmdname, cmd-name);
}
} else {
/* find the command */
-for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
+for (cmd = cmd_table; cmd-name != NULL; cmd++) {
if (compare_cmd(args[0], cmd-name)) {
break;
}
}
if (!cmd-name) {
-goto cleanup;
+return;
}
ptype = next_arg_type(cmd-args_type);
@@ -4321,7 +4304,7 @@ static void monitor_find_completion(Monitor *mon,
}
} else if (!strcmp(cmd-name, help|?)) {
readline_set_completion_index(mon-rs, strlen(str));
-for (cmd = mon-cmd_table; cmd-name != NULL; cmd++) {
+for (cmd = cmd_table; cmd-name != NULL; cmd++) {
cmd_completion(mon, str, cmd-name);
}
}
@@ -4330,6 +4313,36 @@ static void monitor_find_completion(Monitor *mon,
break;
}
}
+}
+
+static void monitor_find_completion(Monitor *mon,
+const char *cmdline)
+{
+char *args[MAX_ARGS];
+int nb_args, len;
+
+/* 1. parse the cmdline */
+if (parse_cmdline(cmdline, nb_args, args) 0) {
+return;
+}
+#ifdef DEBUG_COMPLETION
+for (i = 0; i nb_args; i++) {
+monitor_printf(mon, arg%d = '%s'\n, i, args[i]);
+}
+#endif
+
+/* if the line ends with a space, it means we want to complete the
+ next arg */
+len = strlen(cmdline);
+if (len 0 qemu_isspace(cmdline[len - 1])) {
+if (nb_args = MAX_ARGS) {
+goto cleanup;
+}
+args[nb_args++] = g_strdup();
+}
+
+/* 2. auto complete according to args */
+monitor_find_completion_by_table(mon, mon-cmd_table, args, nb_args);
cleanup:
free_cmdline_args(args, nb_args);
--
1.7.1
[Qemu-devel] [PATCH V9 07/15] monitor: split off monitor_data_init()
In qmp_human_monitor_command(), the monitor need to initialized for
basic functionalities, and later more init code will be added, so
split off this function. Note that it is different with QMP mode
monitor which accept json string from monitor's input,
qmp_human_monitor_command() retrieve the human style command from
QMP input, then send the command to a normal mode monitor.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
monitor.c | 20 +++-
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/monitor.c b/monitor.c
index bf019e2..3ac98ef 100644
--- a/monitor.c
+++ b/monitor.c
@@ -683,14 +683,24 @@ static int do_qmp_capabilities(Monitor *mon, const QDict
*params,
static void handle_user_command(Monitor *mon, const char *cmdline);
+static void monitor_data_init(Monitor *mon)
+{
+memset(mon, 0, sizeof(Monitor));
+mon-outbuf = qstring_new();
+}
+
+static void monitor_data_destroy(Monitor *mon)
+{
+QDECREF(mon-outbuf);
+}
+
char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
int64_t cpu_index, Error **errp)
{
char *output = NULL;
Monitor *old_mon, hmp;
-memset(hmp, 0, sizeof(hmp));
-hmp.outbuf = qstring_new();
+monitor_data_init(hmp);
hmp.skip_flush = true;
old_mon = cur_mon;
@@ -716,7 +726,7 @@ char *qmp_human_monitor_command(const char *command_line,
bool has_cpu_index,
}
out:
-QDECREF(hmp.outbuf);
+monitor_data_destroy(hmp);
return output;
}
@@ -4766,8 +4776,8 @@ void monitor_init(CharDriverState *chr, int flags)
is_first_init = 0;
}
-mon = g_malloc0(sizeof(*mon));
-mon-outbuf = qstring_new();
+mon = g_malloc(sizeof(*mon));
+monitor_data_init(mon);
mon-chr = chr;
mon-flags = flags;
--
1.7.1
[Qemu-devel] [PATCH V9 15/15] monitor: improve auto complete of help for single command in sub group
Now special case help * in auto completion can work with sub commands,
such as help info u*.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/monitor.c b/monitor.c
index b657c85..62fc433 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4330,10 +4330,8 @@ static void monitor_find_completion_by_table(Monitor
*mon,
cmd_completion(mon, str, QKeyCode_lookup[i]);
}
} else if (!strcmp(cmd-name, help|?)) {
-readline_set_completion_index(mon-rs, strlen(str));
-for (cmd = cmd_table; cmd-name != NULL; cmd++) {
-cmd_completion(mon, str, cmd-name);
-}
+monitor_find_completion_by_table(mon, cmd_table,
+ args[1], nb_args - 1);
}
break;
default:
--
1.7.1
[Qemu-devel] [PATCH V9 09/15] monitor: code move for parse_cmdline()
help_cmd() need this function later, so move it. get_str() is called by
parse_cmdline() so it is moved also. Some code style error reported by
check script, is also fixed.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 191 +++--
1 files changed, 98 insertions(+), 93 deletions(-)
diff --git a/monitor.c b/monitor.c
index 8152607..1db0117 100644
--- a/monitor.c
+++ b/monitor.c
@@ -753,6 +753,104 @@ static int compare_cmd(const char *name, const char *list)
return 0;
}
+static int get_str(char *buf, int buf_size, const char **pp)
+{
+const char *p;
+char *q;
+int c;
+
+q = buf;
+p = *pp;
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*p == '\0') {
+fail:
+*q = '\0';
+*pp = p;
+return -1;
+}
+if (*p == '\') {
+p++;
+while (*p != '\0' *p != '\') {
+if (*p == '\\') {
+p++;
+c = *p++;
+switch (c) {
+case 'n':
+c = '\n';
+break;
+case 'r':
+c = '\r';
+break;
+case '\\':
+case '\'':
+case '\':
+break;
+default:
+qemu_printf(unsupported escape code: '\\%c'\n, c);
+goto fail;
+}
+if ((q - buf) buf_size - 1) {
+*q++ = c;
+}
+} else {
+if ((q - buf) buf_size - 1) {
+*q++ = *p;
+}
+p++;
+}
+}
+if (*p != '\') {
+qemu_printf(unterminated string\n);
+goto fail;
+}
+p++;
+} else {
+while (*p != '\0' !qemu_isspace(*p)) {
+if ((q - buf) buf_size - 1) {
+*q++ = *p;
+}
+p++;
+}
+}
+*q = '\0';
+*pp = p;
+return 0;
+}
+
+#define MAX_ARGS 16
+
+/* NOTE: this parser is an approximate form of the real command parser */
+static void parse_cmdline(const char *cmdline,
+ int *pnb_args, char **args)
+{
+const char *p;
+int nb_args, ret;
+char buf[1024];
+
+p = cmdline;
+nb_args = 0;
+for (;;) {
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*p == '\0') {
+break;
+}
+if (nb_args = MAX_ARGS) {
+break;
+}
+ret = get_str(buf, sizeof(buf), p);
+args[nb_args] = g_strdup(buf);
+nb_args++;
+if (ret 0) {
+break;
+}
+}
+*pnb_args = nb_args;
+}
+
static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
const char *prefix, const char *name)
{
@@ -3433,71 +3531,6 @@ static int get_double(Monitor *mon, double *pval, const
char **pp)
return 0;
}
-static int get_str(char *buf, int buf_size, const char **pp)
-{
-const char *p;
-char *q;
-int c;
-
-q = buf;
-p = *pp;
-while (qemu_isspace(*p))
-p++;
-if (*p == '\0') {
-fail:
-*q = '\0';
-*pp = p;
-return -1;
-}
-if (*p == '\') {
-p++;
-while (*p != '\0' *p != '\') {
-if (*p == '\\') {
-p++;
-c = *p++;
-switch(c) {
-case 'n':
-c = '\n';
-break;
-case 'r':
-c = '\r';
-break;
-case '\\':
-case '\'':
-case '\':
-break;
-default:
-qemu_printf(unsupported escape code: '\\%c'\n, c);
-goto fail;
-}
-if ((q - buf) buf_size - 1) {
-*q++ = c;
-}
-} else {
-if ((q - buf) buf_size - 1) {
-*q++ = *p;
-}
-p++;
-}
-}
-if (*p != '\') {
-qemu_printf(unterminated string\n);
-goto fail;
-}
-p++;
-} else {
-while (*p != '\0' !qemu_isspace(*p)) {
-if ((q - buf) buf_size - 1) {
-*q++ = *p;
-}
-p++;
-}
-}
-*q = '\0';
-*pp = p;
-return 0;
-}
-
/*
* Store the command-name in cmdname, and return a pointer to
* the remaining of the command string.
@@ -3554,8 +3587,6 @@ static char *key_get_info(const char *type, char **key)
static int default_fmt_format = 'x';
static int default_fmt_size = 4;
-#define MAX_ARGS 16
-
static int is_valid_option(const char *c, const char *typestr)
{
char option[3
[Qemu-devel] [PATCH V9 14/15] monitor: allow help show message for single command in sub group
A new parameter type 'S' is introduced to allow user input any string.
help info block works normal now.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
hmp-commands.hx |2 +-
monitor.c | 27 +++
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8c6b91a..c161fe9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -11,7 +11,7 @@ ETEXI
{
.name = help|?,
-.args_type = name:s?,
+.args_type = name:S?,
.params = [cmd],
.help = show the help,
.mhandler.cmd = do_help_cmd,
diff --git a/monitor.c b/monitor.c
index 47791b4..b657c85 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,6 +83,7 @@
* 'F' filename
* 'B' block device name
* 's' string (accept optional quote)
+ * 'S' it just appends the rest of the string (accept optional quote)
* 'O' option string of the form NAME=VALUE,...
* parsed according to QemuOptsList given by its name
* Example: 'device:O' uses qemu_device_opts.
@@ -4047,6 +4048,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor
*mon,
}
}
break;
+case 'S':
+{
+/* package all remaining string */
+int len;
+
+while (qemu_isspace(*p)) {
+p++;
+}
+if (*typestr == '?') {
+typestr++;
+if (*p == '\0') {
+/* no remaining string: NULL argument */
+break;
+}
+}
+len = strlen(p);
+if (len = 0) {
+monitor_printf(mon, %s: string expected\n,
+ cmdname);
+break;
+}
+qdict_put(qdict, key, qstring_from_str(p));
+p += len;
+}
+break;
default:
bad_type:
monitor_printf(mon, %s: unknown type '%c'\n, cmdname, c);
@@ -4294,6 +4320,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
bdrv_iterate(block_completion_it, mbs);
break;
case 's':
+case 'S':
if (!strcmp(cmd-name, sendkey)) {
char *sep = strrchr(str, '-');
if (sep)
--
1.7.1
[Qemu-devel] [PATCH V9 13/15] monitor: support sub command in auto completion
This patch allows auto completion work normal for sub command case,
info block [DEVICE] can auto complete now, by re-enter the completion
function. In original code info is treated as a special case, now it
is treated as a sub command group, global variable info_cmds is not used
any more.
help command is still treated as a special case, since it is not a sub
command group but want to auto complete command in root command table.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 14 +++---
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/monitor.c b/monitor.c
index 49a5a88..47791b4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4262,6 +4262,12 @@ static void monitor_find_completion_by_table(Monitor
*mon,
return;
}
+if (cmd-sub_table) {
+/* do the job again */
+return monitor_find_completion_by_table(mon, cmd-sub_table,
+args[1], nb_args - 1);
+}
+
ptype = next_arg_type(cmd-args_type);
for(i = 0; i nb_args - 2; i++) {
if (*ptype != '\0') {
@@ -4288,13 +4294,7 @@ static void monitor_find_completion_by_table(Monitor
*mon,
bdrv_iterate(block_completion_it, mbs);
break;
case 's':
-/* XXX: more generic ? */
-if (!strcmp(cmd-name, info)) {
-readline_set_completion_index(mon-rs, strlen(str));
-for(cmd = info_cmds; cmd-name != NULL; cmd++) {
-cmd_completion(mon, str, cmd-name);
-}
-} else if (!strcmp(cmd-name, sendkey)) {
+if (!strcmp(cmd-name, sendkey)) {
char *sep = strrchr(str, '-');
if (sep)
str = sep + 1;
--
1.7.1
Re: [Qemu-devel] [PATCH V8 00/13] monitor: support sub command group in auto completion and help
于 2013-8-20 22:04, Luiz Capitulino 写道:
On Tue, 30 Jul 2013 12:03:11 -0400
Luiz Capitulino lcapitul...@redhat.com wrote:
On Fri, 26 Jul 2013 11:20:29 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
This series make auto completion and help functions works normal for sub
command, by using reentrant functions. In order to do that, global variables
are not directly used in those functions any more. With this series, cmd_table
is a member of structure Monitor so it is possible to create a monitor with
different command table now, auto completion will work in that monitor. In
short, info is not treated as a special case now, this series ensure help
and auto complete function works normal for any sub command added in the future.
Patch 5 replaced cur_mon with rs-mon, it is safe because:
monitor_init() calls readline_init() which initialize mon-rs, result is
mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make
monitor_read() function take *mon as its opaque. Later, when user input,
monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque.
If qemu's monitors run in one thread, then later in readline_handle_byte()
and readline_comletion(), cur_mon is actually equal to rs-mon, in another
word, it points to the monitor instance, so it is safe to replace *cur_mon
in those functions.
I've applied this to qmp-next with the change I suggested for
patch 09/13.
Unfortunately this series brakes make check:
GTESTER check-qtest-x86_64
Broken pipe
GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
main-loop: WARNING: I/O thread spun for 1000 iterations
make: *** [check-qtest-x86_64] Error 1
I debugged it (with some help from Laszlo) and the problem is that it
broke the human-monitor-command command. Any usage of this command
triggers the bug like:
{ execute: human-monitor-command,
arguments: { command-line: info registers } }
It seems simple to fix, I think you just have to initialize
mon-cmd_table in qmp_human_monitor_command(), but I'd recommend two
things:
1. It's better to split off some/all QMP initialization from
monitor_init() and call it from qmp_human_monitor_command()
2. Can you please take the opportunity and test all commands using
cur_mon? Just grep for it
Sorry for noticing this only now, but I only run make check before
sending a pull request (although this very likely shows you didn't
run it either).
About the fd related qmp interface, to test it, send_msg() is needed,
which was not supported in python 2, but new added python 3.3. I think
there are three ways to add test cases for fd qmp APIs:
1 test only when python 3.3.
2 python plus C: compile a .so and call it with ctypes.
3 a new test framework: pure C code to call qmp interfaces.
Which one do you prefer?
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v5 0/8] Implement reference count for BlockDriverState [resend]
于 2013-8-22 19:38, Stefan Hajnoczi 写道: On Fri, Aug 09, 2013 at 06:01:53PM +0800, Fam Zheng wrote: [resend to the correct list] BlockDriverState lifecycle management is needed by future features such as image fleecing and blockdev-add. This series adds reference count to BlockDriverState. The first two patches clean up two odd BlockDriverState use cases, so all code uses bdrv_new() to create BlockDriverState instance. Then implemented bdrv_ref() and bdrv_unref() to operate on refcnt: Initially, refcnt is 1, which means bdrv_unref is effectively a bdrv_delete() here. So patch 04 has a search and replace to convert bdrv_delete to bdrv_unref, before bdrv_ref is used anywhere. 05~08 patches calls bdrv_ref for device attach, block-migration and nbd. The rule is: Either bdrv_ref() or bdrv_new() must have a matching bdrv_unref() call, and the last matching bdrv_unref deletes the bs. v4: 08: Added, let block job use BDS reference. 02: Fix leak of bs.opaque v3: 03: Removed unnecessary bdrv_close() call. v2: 05: Removed: block: use BlockDriverState refcnt for device attach/detach 07: Fix xen_disk blk_disconnect() as it depended on device attach refcnt. Sorry, can't merge this because it breaks qemu-iotests 041 and 055: $ ./check -qcow2 055 041 Please always run qemu-iotests before submitting patches. Stefan Hi, What is the correct steps to run full qemu-iotests? I modified qemu-iotests-quick.sh as: #!/bin/sh # We don't know which of the system emulator binaries there is (or if there is # any at all), so the 'quick' group doesn't contain any tests that require # running qemu proper. Assign a fake binary name so that qemu-iotests doesn't # complain about the missing binary. export QEMU_PROG=$(pwd)/x86_64-softmmu/qemu-system-x86_64 export QEMU_IMG_PROG=$(pwd)/qemu-img export QEMU_IO_PROG=$(pwd)/qemu-io export QEMU_NBD_PROG=$(pwd)/qemu-nbd cd $SRC_PATH/tests/qemu-iotests ret=0 ./check -T -nocache -qcow2 || ret=1 exit $ret Then make check-block, 026 038 fail, 038 sometimes fail. The code from is upstream, host is RH6.3 @ x86_64. Do I missed some steps? -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V8 00/13] monitor: support sub command group in auto completion and help
于 2013-8-20 22:04, Luiz Capitulino 写道:
On Tue, 30 Jul 2013 12:03:11 -0400
Luiz Capitulino lcapitul...@redhat.com wrote:
On Fri, 26 Jul 2013 11:20:29 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
This series make auto completion and help functions works normal for sub
command, by using reentrant functions. In order to do that, global variables
are not directly used in those functions any more. With this series, cmd_table
is a member of structure Monitor so it is possible to create a monitor with
different command table now, auto completion will work in that monitor. In
short, info is not treated as a special case now, this series ensure help
and auto complete function works normal for any sub command added in the future.
Patch 5 replaced cur_mon with rs-mon, it is safe because:
monitor_init() calls readline_init() which initialize mon-rs, result is
mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make
monitor_read() function take *mon as its opaque. Later, when user input,
monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque.
If qemu's monitors run in one thread, then later in readline_handle_byte()
and readline_comletion(), cur_mon is actually equal to rs-mon, in another
word, it points to the monitor instance, so it is safe to replace *cur_mon
in those functions.
I've applied this to qmp-next with the change I suggested for
patch 09/13.
Unfortunately this series brakes make check:
GTESTER check-qtest-x86_64
Broken pipe
GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
main-loop: WARNING: I/O thread spun for 1000 iterations
make: *** [check-qtest-x86_64] Error 1
I debugged it (with some help from Laszlo) and the problem is that it
broke the human-monitor-command command. Any usage of this command
triggers the bug like:
{ execute: human-monitor-command,
arguments: { command-line: info registers } }
It seems simple to fix, I think you just have to initialize
mon-cmd_table in qmp_human_monitor_command(), but I'd recommend two
things:
1. It's better to split off some/all QMP initialization from
monitor_init() and call it from qmp_human_monitor_command()
2. Can you please take the opportunity and test all commands using
cur_mon? Just grep for it
Sorry for noticing this only now, but I only run make check before
sending a pull request (although this very likely shows you didn't
run it either).
My bad that not ran make check before, will fix and retry, sorry for
the trouble.
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes
于 2013-8-21 16:45, Stefan Hajnoczi 写道:
On Tue, Aug 20, 2013 at 05:59:58PM +0800, Wenchao Xia wrote:
于 2013-8-16 16:12, Wenchao Xia 写道:
于 2013-8-16 15:15, Wenchao Xia 写道:
于 2013-8-16 0:32, Michael Roth 写道:
Quoting Michael Roth (2013-08-15 10:23:20)
Quoting Wenchao Xia (2013-08-13 03:44:39)
于 2013-8-13 1:01, Michael Roth 写道:
Quoting Paolo Bonzini (2013-08-12 02:30:28)
1) rename AioContext to AioSource.
This is my major purpose, which declare it is not a context
concept,
and GMainContext is the entity represent the thread's activity.
Note that the nested event loops in QEMU are _very_ different from
glib nested event loops. In QEMU, nested event loops only run block
layer events. In glib, they run all events. That's why you need
AioContext.
Would it be possible to use glib for our nested loops as well by just
setting a higher priority for the AioContext GSource?
Stefan and I were considering how we could make use of his drop
ioflush patches to use a common mechanism to register fd events, but
still allow us to distinguish between AioContext and non-AioContext
for nested loops. I was originally thinking of using prepare()
functions
to filter out non-AioContext events, but that requires we implement
on GSource's with that in mind, and non make use of pre-baked ones
like GIOChannel's, and bakes block stuff into every event source
implementation.
Besides priority, also g_source_set_can_recurse() can help.
With a deeper think, I found a harder problem:
g_main_context_acquire() and g_main_context_release(). In release,
pending BH/IO call back need to be cleared, but this action can't
be triggered automatically when user call g_main_context_release().
I don't understand why this is a requirement, gmctx_acquire/release
ensure
that only one thread attempts to iterate the main loop at a time. this
isn't currently an issue in qemu, and if we re-implemented
qemu_aio_wait()
to use the same glib interfaces, the tracking of in-flight requests
would
be moved to the block layer via Stefan's 'drop io_flush' patches, which
moves that block-specific logic out of the main loop/AioContext GSource
by design. Are there other areas where you see this as a problem?
I think I understand better what you're referring to, you mean that
if qemu_aio_wait was called, and was implementated to essentially call
g_main_context_iterate(), that after, say, 1 iteration, the
iothread/dataplane thread might acquire the main loop and dispatch
block/non-block events between qemu_aio_wait() returned? The simple
approach would be to have qemu_aio_wait() call
g_main_context_acquire/release
at the start end of the function, which would ensure that this never
happens.
qemu_aio_wait() is relative simple to resolve by
g_main_context_acquire(), but also shows additional code needed
for a thread switch:
(guess following is the plan to implement qemu_aio_wait())
qemu_aio_wait(GMainContext *ctx)
{
return g_main_context(ctx, PRI_BLOCK);
}
at caller:
{
while (qemu_aio_wait(ctx) {
;
}
g_main_context_release(ctx);
}
The above code shows that, in *ctx switch operation, there is
more than glib's own event loop API envolved, qemu_aio_wait(). So
it referenced to a question: what data structure
should be used to represent context concept and control the thread
switching behavior? It is better to have a clear layer, GMainContext or
GlibQContext, instead of GMainContext plus custom function. The caller
reference to at least two: nested user block layer, flat user above
block layer.
In my opinion, this problem is brought by Gsource AioContext, Take
the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
an example, there are aync following operations involved for AioContext:
1 the bdrv_cb() will be executed in bdrv_co_em_bh().
2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
3 aio_worker() will be executed in worker_thread().
Operation 2 and 3 are tracked by block layer's queue after Stefan's
dropping io_flush() series.
Now if we stick to GMainContext to represent context concept,
then when thread B want to aquire GMainContext used by thread A,
all works setupped by A should be finished before B aquire it,
otherwise B will execute some function supposed to work in A. The
work refers to the three kind of aync functions above.
For this issue, we can solve it by different means:
1 the event loop API doesn't guarentee work setupped by thread A
will always be finished in A. This put a limitation to caller to
consider thread switching. I talked on IRC with Stefan, and thinks
it is possible for the nested user block layer, but I still want to
avoid this to the flat user above block layer.
2 ask caller to finish all pending operations.
2.1 glib GMainContext API plus custom API such as aio_wait(). This is
bad that detail under GMainContext is exposed to caller. Since
operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
is called in thread setupped
Re: [Qemu-devel] [PATCHv11 13/31] aio / timers: aio_ctx_prepare sets timeout from AioContext timers
于 2013-8-20 14:48, Alex Bligh 写道:
for (bh = ctx-first_bh; bh; bh = bh-next) {
if (!bh-deleted bh-scheduled) {
if (bh-idle) {
/* idle bottom halves will be polled at least
* every 10ms */
-*timeout = 10;
+*timeout = qemu_soonest_timeout(*timeout, 10);
glib will not set *timeout to a meaningful value before calling
aio_ctx_prepare(), right? If so, *timeout = 10 should be used.
I don't think that's correct. Each _prepare routine is called
and has the abilitiy to alter the timeout but need not
set it at all. Indeed it should not set it as there may already
be a shorter timeout there.
Here's the code before I touched it:
aio_ctx_prepare(GSource *source, gint*timeout)
{
AioContext *ctx = (AioContext *) source;
QEMUBH *bh;
for (bh = ctx-first_bh; bh; bh = bh-next) {
if (!bh-deleted bh-scheduled) {
if (bh-idle) {
/* idle bottom halves will be polled at least
* every 10ms */
*timeout = 10;
} else {
/* non-idle bottom halves will be executed
* immediately */
*timeout = 0;
return true;
}
}
}
return false;
}
You'll note that what this does is:
a) if there are no bottom halves, leaves *timeout as is
b) if there is a non-idle bottom half, set timeout to 0
c) else set timeout to 10 if there are only idle bottom
halves.
Arguably (c) is a bug if timeout was shorter than 10
on entry but the whole of idle bhs are a bit of a bodge.
This is fixed by my series.
If you are asking WHERE it gets set to -1, I don't claim
to be a glib expert but I believe it's the line
gint source_timeout = -1
around line 2816 in glib/gmain.c
Thanks for the explanation. It seems *timeout is always set
to -1 before calling GSource's prepare(), so
*timeout = qemu_soonest_timeout(*timeout, 10);
will always get *timeout = 10, so this call can be saved.
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes
于 2013-8-16 16:12, Wenchao Xia 写道:
于 2013-8-16 15:15, Wenchao Xia 写道:
于 2013-8-16 0:32, Michael Roth 写道:
Quoting Michael Roth (2013-08-15 10:23:20)
Quoting Wenchao Xia (2013-08-13 03:44:39)
于 2013-8-13 1:01, Michael Roth 写道:
Quoting Paolo Bonzini (2013-08-12 02:30:28)
1) rename AioContext to AioSource.
This is my major purpose, which declare it is not a context
concept,
and GMainContext is the entity represent the thread's activity.
Note that the nested event loops in QEMU are _very_ different from
glib nested event loops. In QEMU, nested event loops only run block
layer events. In glib, they run all events. That's why you need
AioContext.
Would it be possible to use glib for our nested loops as well by just
setting a higher priority for the AioContext GSource?
Stefan and I were considering how we could make use of his drop
ioflush patches to use a common mechanism to register fd events, but
still allow us to distinguish between AioContext and non-AioContext
for nested loops. I was originally thinking of using prepare()
functions
to filter out non-AioContext events, but that requires we implement
on GSource's with that in mind, and non make use of pre-baked ones
like GIOChannel's, and bakes block stuff into every event source
implementation.
Besides priority, also g_source_set_can_recurse() can help.
With a deeper think, I found a harder problem:
g_main_context_acquire() and g_main_context_release(). In release,
pending BH/IO call back need to be cleared, but this action can't
be triggered automatically when user call g_main_context_release().
I don't understand why this is a requirement, gmctx_acquire/release
ensure
that only one thread attempts to iterate the main loop at a time. this
isn't currently an issue in qemu, and if we re-implemented
qemu_aio_wait()
to use the same glib interfaces, the tracking of in-flight requests
would
be moved to the block layer via Stefan's 'drop io_flush' patches, which
moves that block-specific logic out of the main loop/AioContext GSource
by design. Are there other areas where you see this as a problem?
I think I understand better what you're referring to, you mean that
if qemu_aio_wait was called, and was implementated to essentially call
g_main_context_iterate(), that after, say, 1 iteration, the
iothread/dataplane thread might acquire the main loop and dispatch
block/non-block events between qemu_aio_wait() returned? The simple
approach would be to have qemu_aio_wait() call
g_main_context_acquire/release
at the start end of the function, which would ensure that this never
happens.
qemu_aio_wait() is relative simple to resolve by
g_main_context_acquire(), but also shows additional code needed
for a thread switch:
(guess following is the plan to implement qemu_aio_wait())
qemu_aio_wait(GMainContext *ctx)
{
return g_main_context(ctx, PRI_BLOCK);
}
at caller:
{
while (qemu_aio_wait(ctx) {
;
}
g_main_context_release(ctx);
}
The above code shows that, in *ctx switch operation, there is
more than glib's own event loop API envolved, qemu_aio_wait(). So
it referenced to a question: what data structure
should be used to represent context concept and control the thread
switching behavior? It is better to have a clear layer, GMainContext or
GlibQContext, instead of GMainContext plus custom function. The caller
reference to at least two: nested user block layer, flat user above
block layer.
In my opinion, this problem is brought by Gsource AioContext, Take
the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
an example, there are aync following operations involved for AioContext:
1 the bdrv_cb() will be executed in bdrv_co_em_bh().
2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
3 aio_worker() will be executed in worker_thread().
Operation 2 and 3 are tracked by block layer's queue after Stefan's
dropping io_flush() series.
Now if we stick to GMainContext to represent context concept,
then when thread B want to aquire GMainContext used by thread A,
all works setupped by A should be finished before B aquire it,
otherwise B will execute some function supposed to work in A. The
work refers to the three kind of aync functions above.
For this issue, we can solve it by different means:
1 the event loop API doesn't guarentee work setupped by thread A
will always be finished in A. This put a limitation to caller to
consider thread switching. I talked on IRC with Stefan, and thinks
it is possible for the nested user block layer, but I still want to
avoid this to the flat user above block layer.
2 ask caller to finish all pending operations.
2.1 glib GMainContext API plus custom API such as aio_wait(). This is
bad that detail under GMainContext is exposed to caller. Since
operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
is called in thread setupped it, 1 need to be added in the track
queue, or in the call chain of aio_wait(), check the queue plus
Re: [Qemu-devel] [PATCHv11 13/31] aio / timers: aio_ctx_prepare sets timeout from AioContext timers
于 2013-8-20 17:29, Alex Bligh 写道: On 20 Aug 2013, at 08:19, Wenchao Xia wrote: Thanks for the explanation. It seems *timeout is always set to -1 before calling GSource's prepare(), so *timeout = qemu_soonest_timeout(*timeout, 10); will always get *timeout = 10, so this call can be saved. I believe that's incorrect too. glib *currently* calls the API that way, but there's nothing to say a future or past glub couldn't call each g_source with the same timeout pointer, with each g_source adjusting the timeout downwards. Or (for instance) call it with 0 for a So it is an undefined value, should avoid use it? For example, other gsource have minimum timeout of 5, and aio_ctx_prepare() is called with 0, then returned timeout is 0, but it is supposed to work with timeout 5 as the glib doc described: https://developer.gnome.org/glib/2.32/glib-The-Main-Event-Loop.html#GSourceFuncs That is my opinion, but I haven't read other project's code about prepare() usage, so can't be sure. Guess maintainer can give a conclusion. non-blocking prepare. Therefore the implemented way is safer (only reducing the timeout). -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCHv11 13/31] aio / timers: aio_ctx_prepare sets timeout from AioContext timers
于 2013-8-16 4:34, Alex Bligh 写道:
Calculate the timeout in aio_ctx_prepare taking into account
the timers attached to the AioContext.
Alter aio_ctx_check similarly.
Signed-off-by: Alex Bligh a...@alex.org.uk
---
async.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/async.c b/async.c
index 2b9ba9b..d8656cc 100644
--- a/async.c
+++ b/async.c
@@ -150,13 +150,14 @@ aio_ctx_prepare(GSource *source, gint*timeout)
{
AioContext *ctx = (AioContext *) source;
QEMUBH *bh;
+int deadline;
for (bh = ctx-first_bh; bh; bh = bh-next) {
if (!bh-deleted bh-scheduled) {
if (bh-idle) {
/* idle bottom halves will be polled at least
* every 10ms */
-*timeout = 10;
+*timeout = qemu_soonest_timeout(*timeout, 10);
glib will not set *timeout to a meaningful value before calling
aio_ctx_prepare(), right? If so, *timeout = 10 should be used.
} else {
/* non-idle bottom halves will be executed
* immediately */
@@ -166,6 +167,14 @@ aio_ctx_prepare(GSource *source, gint*timeout)
}
}
+deadline = qemu_timeout_ns_to_ms(timerlistgroup_deadline_ns(ctx-tlg));
+if (deadline == 0) {
+*timeout = 0;
+return true;
+} else {
+*timeout = qemu_soonest_timeout(*timeout, deadline);
+}
+
return false;
}
@@ -180,7 +189,7 @@ aio_ctx_check(GSource *source)
return true;
}
}
-return aio_pending(ctx);
+return aio_pending(ctx) || (timerlistgroup_deadline_ns(ctx-tlg) == 0);
}
static gboolean
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes
于 2013-8-16 0:32, Michael Roth 写道:
Quoting Michael Roth (2013-08-15 10:23:20)
Quoting Wenchao Xia (2013-08-13 03:44:39)
于 2013-8-13 1:01, Michael Roth 写道:
Quoting Paolo Bonzini (2013-08-12 02:30:28)
1) rename AioContext to AioSource.
This is my major purpose, which declare it is not a context concept,
and GMainContext is the entity represent the thread's activity.
Note that the nested event loops in QEMU are _very_ different from
glib nested event loops. In QEMU, nested event loops only run block
layer events. In glib, they run all events. That's why you need
AioContext.
Would it be possible to use glib for our nested loops as well by just
setting a higher priority for the AioContext GSource?
Stefan and I were considering how we could make use of his drop
ioflush patches to use a common mechanism to register fd events, but
still allow us to distinguish between AioContext and non-AioContext
for nested loops. I was originally thinking of using prepare() functions
to filter out non-AioContext events, but that requires we implement
on GSource's with that in mind, and non make use of pre-baked ones
like GIOChannel's, and bakes block stuff into every event source
implementation.
Besides priority, also g_source_set_can_recurse() can help.
With a deeper think, I found a harder problem:
g_main_context_acquire() and g_main_context_release(). In release,
pending BH/IO call back need to be cleared, but this action can't
be triggered automatically when user call g_main_context_release().
I don't understand why this is a requirement, gmctx_acquire/release ensure
that only one thread attempts to iterate the main loop at a time. this
isn't currently an issue in qemu, and if we re-implemented qemu_aio_wait()
to use the same glib interfaces, the tracking of in-flight requests would
be moved to the block layer via Stefan's 'drop io_flush' patches, which
moves that block-specific logic out of the main loop/AioContext GSource
by design. Are there other areas where you see this as a problem?
I think I understand better what you're referring to, you mean that
if qemu_aio_wait was called, and was implementated to essentially call
g_main_context_iterate(), that after, say, 1 iteration, the
iothread/dataplane thread might acquire the main loop and dispatch
block/non-block events between qemu_aio_wait() returned? The simple
approach would be to have qemu_aio_wait() call g_main_context_acquire/release
at the start end of the function, which would ensure that this never
happens.
qemu_aio_wait() is relative simple to resolve by
g_main_context_acquire(), but also shows additional code needed
for a thread switch:
(guess following is the plan to implement qemu_aio_wait())
qemu_aio_wait(GMainContext *ctx)
{
return g_main_context(ctx, PRI_BLOCK);
}
at caller:
{
while (qemu_aio_wait(ctx) {
;
}
g_main_context_release(ctx);
}
The above code shows that, in *ctx switch operation, there is
more than glib's own event loop API envolved, qemu_aio_wait(). So
it referenced to a question: what data structure
should be used to represent context concept and control the thread
switching behavior? It is better to have a clear layer, GMainContext or
GlibQContext, instead of GMainContext plus custom function. The caller
reference to at least two: nested user block layer, flat user above
block layer.
In my opinion, this problem is brought by Gsource AioContext, Take
the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
an example, there are aync following operations involved for AioContext:
1 the bdrv_cb() will be executed in bdrv_co_em_bh().
2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
3 aio_worker() will be executed in worker_thread().
Operation 2 and 3 are tracked by block layer's queue after Stefan's
dropping io_flush() series.
Now if we stick to GMainContext to represent context concept,
then when thread B want to aquire GMainContext used by thread A,
all works setupped by A should be finished before B aquire it,
otherwise B will execute some function supposed to work in A. The
work refers to the three kind of aync functions above.
For this issue, we can solve it by different means:
1 the event loop API doesn't guarentee work setupped by thread A
will always be finished in A. This put a limitation to caller to
consider thread switching. I talked on IRC with Stefan, and thinks
it is possible for the nested user block layer, but I still want to
avoid this to the flat user above block layer.
2 ask caller to finish all pending operations.
2.1 glib GMainContext API plus custom API such as aio_wait(). This is
bad that detail under GMainContext is exposed to caller. Since
operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
is called in thread setupped it, 1 need to be added in the track
queue, or in the call chain of aio_wait(), check the queue plus check
operation 1. Perhaps add a custom function ask caller to call
Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes
于 2013-8-16 15:15, Wenchao Xia 写道:
于 2013-8-16 0:32, Michael Roth 写道:
Quoting Michael Roth (2013-08-15 10:23:20)
Quoting Wenchao Xia (2013-08-13 03:44:39)
于 2013-8-13 1:01, Michael Roth 写道:
Quoting Paolo Bonzini (2013-08-12 02:30:28)
1) rename AioContext to AioSource.
This is my major purpose, which declare it is not a context
concept,
and GMainContext is the entity represent the thread's activity.
Note that the nested event loops in QEMU are _very_ different from
glib nested event loops. In QEMU, nested event loops only run block
layer events. In glib, they run all events. That's why you need
AioContext.
Would it be possible to use glib for our nested loops as well by just
setting a higher priority for the AioContext GSource?
Stefan and I were considering how we could make use of his drop
ioflush patches to use a common mechanism to register fd events, but
still allow us to distinguish between AioContext and non-AioContext
for nested loops. I was originally thinking of using prepare()
functions
to filter out non-AioContext events, but that requires we implement
on GSource's with that in mind, and non make use of pre-baked ones
like GIOChannel's, and bakes block stuff into every event source
implementation.
Besides priority, also g_source_set_can_recurse() can help.
With a deeper think, I found a harder problem:
g_main_context_acquire() and g_main_context_release(). In release,
pending BH/IO call back need to be cleared, but this action can't
be triggered automatically when user call g_main_context_release().
I don't understand why this is a requirement, gmctx_acquire/release
ensure
that only one thread attempts to iterate the main loop at a time. this
isn't currently an issue in qemu, and if we re-implemented
qemu_aio_wait()
to use the same glib interfaces, the tracking of in-flight requests
would
be moved to the block layer via Stefan's 'drop io_flush' patches, which
moves that block-specific logic out of the main loop/AioContext GSource
by design. Are there other areas where you see this as a problem?
I think I understand better what you're referring to, you mean that
if qemu_aio_wait was called, and was implementated to essentially call
g_main_context_iterate(), that after, say, 1 iteration, the
iothread/dataplane thread might acquire the main loop and dispatch
block/non-block events between qemu_aio_wait() returned? The simple
approach would be to have qemu_aio_wait() call
g_main_context_acquire/release
at the start end of the function, which would ensure that this never
happens.
qemu_aio_wait() is relative simple to resolve by
g_main_context_acquire(), but also shows additional code needed
for a thread switch:
(guess following is the plan to implement qemu_aio_wait())
qemu_aio_wait(GMainContext *ctx)
{
return g_main_context(ctx, PRI_BLOCK);
}
at caller:
{
while (qemu_aio_wait(ctx) {
;
}
g_main_context_release(ctx);
}
The above code shows that, in *ctx switch operation, there is
more than glib's own event loop API envolved, qemu_aio_wait(). So
it referenced to a question: what data structure
should be used to represent context concept and control the thread
switching behavior? It is better to have a clear layer, GMainContext or
GlibQContext, instead of GMainContext plus custom function. The caller
reference to at least two: nested user block layer, flat user above
block layer.
In my opinion, this problem is brought by Gsource AioContext, Take
the call path of bdrv_aio_readv(*bdrv_cb) on raw linux file as
an example, there are aync following operations involved for AioContext:
1 the bdrv_cb() will be executed in bdrv_co_em_bh().
2 bdrv_co_io_em_complete() will be executed in event_notfier_ready().
3 aio_worker() will be executed in worker_thread().
Operation 2 and 3 are tracked by block layer's queue after Stefan's
dropping io_flush() series.
Now if we stick to GMainContext to represent context concept,
then when thread B want to aquire GMainContext used by thread A,
all works setupped by A should be finished before B aquire it,
otherwise B will execute some function supposed to work in A. The
work refers to the three kind of aync functions above.
For this issue, we can solve it by different means:
1 the event loop API doesn't guarentee work setupped by thread A
will always be finished in A. This put a limitation to caller to
consider thread switching. I talked on IRC with Stefan, and thinks
it is possible for the nested user block layer, but I still want to
avoid this to the flat user above block layer.
2 ask caller to finish all pending operations.
2.1 glib GMainContext API plus custom API such as aio_wait(). This is
bad that detail under GMainContext is exposed to caller. Since
operation 1 mentioned before is not tracked yet, to make sure bdrv_cb()
is called in thread setupped it, 1 need to be added in the track
queue, or in the call chain of aio_wait(), check the queue plus check
operation 1. Perhaps add a custom
Re: [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource
,
+ IOCanReadHandler *fd_read_poll,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)
{
-IOHandlerRecord *ioh;
-
-QLIST_FOREACH(ioh, io_handlers, next) {
-int events = 0;
-
-if (ioh-deleted)
-continue;
-if (ioh-fd_read
-(!ioh-fd_read_poll ||
- ioh-fd_read_poll(ioh-opaque) != 0)) {
-events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
-}
-if (ioh-fd_write) {
-events |= G_IO_OUT | G_IO_ERR;
-}
-if (events) {
-GPollFD pfd = {
-.fd = ioh-fd,
-.events = events,
-};
-ioh-pollfds_idx = pollfds-len;
-g_array_append_val(pollfds, pfd);
-} else {
-ioh-pollfds_idx = -1;
-}
-}
+return set_fd_handler2(qemu_get_qcontext(), fd,
+ fd_read_poll, fd_read, fd_write,
+ opaque);
}
-void qemu_iohandler_poll(GArray *pollfds, int ret)
+int qemu_set_fd_handler(int fd,
+IOHandler *fd_read,
+IOHandler *fd_write,
+void *opaque)
{
-if (ret 0) {
-IOHandlerRecord *pioh, *ioh;
-
-QLIST_FOREACH_SAFE(ioh, io_handlers, next, pioh) {
-int revents = 0;
-
-if (!ioh-deleted ioh-pollfds_idx != -1) {
-GPollFD *pfd = g_array_index(pollfds, GPollFD,
- ioh-pollfds_idx);
-revents = pfd-revents;
-}
-
-if (!ioh-deleted ioh-fd_read
-(revents (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
-ioh-fd_read(ioh-opaque);
-}
-if (!ioh-deleted ioh-fd_write
-(revents (G_IO_OUT | G_IO_ERR))) {
-ioh-fd_write(ioh-opaque);
-}
-
-/* Do this last in case read/write handlers marked it for deletion
*/
-if (ioh-deleted) {
-QLIST_REMOVE(ioh, next);
-g_free(ioh);
-}
-}
-}
+return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
}
/* reaping of zombies. right now we're not passing the status to
diff --git a/main-loop.c b/main-loop.c
index f46aece..ae284a6 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -27,6 +27,8 @@
#include slirp/slirp.h
#include qemu/main-loop.h
#include block/aio.h
+#include qcontext/qcontext.h
+#include qcontext/glib-qcontext.h
#ifndef _WIN32
@@ -107,6 +109,13 @@ static int qemu_signal_init(void)
}
#endif
+static QContext *qemu_qcontext;
+
+QContext *qemu_get_qcontext(void)
+{
+return qemu_qcontext;
+}
+
static AioContext *qemu_aio_context;
AioContext *qemu_get_aio_context(void)
@@ -128,6 +137,7 @@ int qemu_init_main_loop(void)
{
int ret;
GSource *src;
+Error *err = NULL;
init_clocks();
if (init_timer_alarm() 0) {
@@ -135,6 +145,15 @@ int qemu_init_main_loop(void)
exit(1);
}
+qemu_qcontext = QCONTEXT(glib_qcontext_new(main, false, err));
+if (err) {
+g_warning(error initializing main qcontext);
+error_free(err);
+return -1;
+}
+
+iohandler_attach(qemu_qcontext);
+
ret = qemu_signal_init();
if (ret) {
return ret;
@@ -464,9 +483,7 @@ int main_loop_wait(int nonblocking)
slirp_update_timeout(timeout);
slirp_pollfds_fill(gpollfds);
#endif
-qemu_iohandler_fill(gpollfds);
ret = os_host_main_loop_wait(timeout);
-qemu_iohandler_poll(gpollfds, ret);
#ifdef CONFIG_SLIRP
slirp_pollfds_poll(gpollfds, (ret 0));
#endif
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH] Introduce cache images for the QCOW2 format
于 2013-8-14 23:32, Kevin Wolf 写道: Am 14.08.2013 um 16:26 hat Kaveh Razavi geschrieben: On 08/14/2013 03:50 PM, Alex Bligh wrote: Assuming the cache quota is not exhausted, how do you know how that a VM has finished 'creating' the cache? At any point it might read a bit more from the backing image. I was assuming on shutdown. Wait, so you're not really changing the cache while it's used, but you only create it once and then use it like a regular backing file? If so, the only thing we need to talk about is the creation, because there's no difference for using it. Creation can use the existing copy-on-read functionality, and the only thing you need additionally is a way to turn copy-on-read off at the right point. Or do I misunderstand what you're doing? Kevin This cache capability seems have little to do with qcow2, but a general block function: start/stop copy on read for one BS in a backing chain. If so, suggest: 1 refine existing general copy on read code, not in qcow2.c but general block code, make it able to start/stop copy on read for a BDS in the chain. 2 add qmp interface for it. Then the work flow will be: step 1: prepare image, not related to qcow2 format. qemu-img create cache.img -b base.img qemu-img create vm1.img -b cache.img step 2: boot vm1, vm2: qemu -hda vm1.img -COR cache.img qemu -hda vm2.img -- Best Regards Wenchao Xia
Re: [Qemu-devel] Are there plans to achieve ram live Snapshot feature?
于 2013-8-15 15:49, Stefan Hajnoczi 写道: On Thu, Aug 15, 2013 at 10:26:36AM +0800, Wenchao Xia wrote: 于 2013-8-14 15:53, Stefan Hajnoczi 写道: On Wed, Aug 14, 2013 at 3:54 AM, Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-13 16:21, Stefan Hajnoczi 写道: On Tue, Aug 13, 2013 at 4:53 AM, Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-12 19:33, Stefan Hajnoczi 写道: On Mon, Aug 12, 2013 at 12:26 PM, Alex Bligh a...@alex.org.uk wrote: --On 12 August 2013 11:59:03 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: The idea that was discussed on qemu-devel@nongnu.org uses fork(2) to capture the state of guest RAM and then send it back to the parent process. The guest is only paused for a brief instant during fork(2) and can continue to run afterwards. How would you capture the state of emulated hardware which might not be in the guest RAM? Exactly the same way vmsave works today. It calls the device's save functions which serialize state to file. The difference between today's vmsave and the fork(2) approach is that QEMU does not need to wait for guest RAM to be written to file before resuming the guest. Stefan I have a worry about what glib says: On Unix, the GLib mainloop is incompatible with fork(). Any program using the mainloop must either exec() or exit() from the child without returning to the mainloop. This is fine, the child just writes out the memory pages and exits. It never returns to the glib mainloop. There is another way to do it: intercept the write in kvm.ko(or other kernel code). Since the key is intercept the memory change, we can do it in userspace in TCG mode, thus we can add the missing part in KVM mode. Another benefit of this way is: the used memory can be controlled. For example, with ioctl(), set a buffer of a fixed size which keeps the intercepted write data by kernel code, which can avoid frequently switch back to user space qemu code. when it is full always return back to userspace's qemu code, let qemu code save the data into disk. I haven't check the exactly behavior of Intel guest mode about how to handle page fault, so can't estimate the performance caused by switching of guest mode and root mode, but it should not be worse than fork(). The fork(2) approach is portable, covers both KVM and TCG, and doesn't require kernel changes. A kvm.ko kernel change also won't be supported on existing KVM hosts. These are big drawbacks and the kernel approach would need to be significantly better than plain old fork(2) to make it worthwhile. Stefan I think advantage is memory usage is predictable, so memory usage peak can be avoided, by always save the changed pages first. fork() does not know which pages are changed. I am not sure if this would be a serious issue when server's memory is consumed much, for example, 24G host emulate 11G*2 guest to provide powerful virtual server. Memory usage is predictable but guest uptime is unpredictable because it waits until memory is written out. This defeats the point of live savevm. The guest may be stalled arbitrarily. I think it is adjustable. There is no much difference with fork(), except get more precise control about the changed pages. Kernel intercept the change, and stores the changed page in another page, similar to fork(). When userspace qemu code execute, save some pages to disk. Buffer can be used like some lubricant. When Buffer = MAX, it equals to fork(), guest runs more lively. When Buffer = 0, guest runs less lively. I think it allows user to find a good balance point with a parameter. It is harder to implement, just want to show the idea. You are right. You could set a bigger buffer size to increase guest uptime. The fork child can minimize the chance of out-of-memory by using madvise(MADV_DONTNEED) after pages have been written out. It seems no way to make sure the written out page is the changed pages, so it have a good chance the written one is the unchanged and still used by the other qemu process. The KVM dirty log tells you which pages were touched. The fork child process could give priority to the pages which have been touched by the guest. They must be written out and marked madvise(MADV_DONTNEED) as soon as possible. Hmm, if dirty log still works normal in child process to reflect the memory status in parent not child's, then the problem could be solved by: when dirty pages is too much, child tell parent to wait some time. But I haven't check if kvm.ko behaviors like that. I haven't looked at the vmsave data format yet to see if memory pages can be saved in random order, but this might work. It reduces the likelihood of copy-on-write memory growth. Stefan -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
于 2013-8-7 11:00, Wenchao Xia 写道: This series brings internal snapshot support at block devices level, now we have two three methods to do block snapshot lively: 1) backing chain, 2) internal one and 3) drive-back up approach. Comparation: Advantages:Disadvantages: 1)delta data, taken fast, export, sizeperformance, delete slow. 2) taken fast, delete fast, performance, size delta data, format 3) performance, export, format taken slow, delta data, size, host I/O I think in most case, saving vmstate in an standalone file is better than saving it inside qcow2, So suggest treat internal snapshot as block level methods and not encourage user to savevm in qcow2 any more. Implemention details: To avoid trouble, this serial have hide ID in create interfaces, this make sure no chaos of ID and name will be introduced by these interfaces. There is one patch may be common to Pavel's savvm transaction, patch 1/11, others are not quite related. Patch 1/11 will not set errp when no snapshot find, since patch 3/11 need to distinguish real error case. Next steps to better full VM snapshot: Improve internal snapshot's export capability. Better vmstate saving. Thanks Kevin to give advisement about how add it in qmp_transaction, oldest version comes drom Dietmar Maurer. v3: General: Rebased after Stenfan's driver-backup patch V6. Address Eric's comments: 4/9: grammar fix and better doc. 5/9: parameter name is mandatory now. grammar fix. 6/9: redesiged interface: take both id and name as optional parameter, return the deleted snapshot's info. Address Stefan's comments: 4/9: add '' around %s in message. drop code comments about vm_clock. 9/9: better doc, refined the code and add more test case. v4: Address Stefan's comments: 4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(), spell fix and better doc. 5/9: better doc. 6/9: remove spurious ';' in code, spell fix and better doc. v5: Address Kevin's comments: 3/8, 4/8, 8/8: remove the limit of numeric snapshot name. General change: 4/8: use existing type as parameter in qapi schema. v6: Address Stefan's comments: 2/8: macro STR_PRINT_CHAR was renamed as STR_OR_NULL, and moved into patch 5, since implement function in this patch do not printf snapshot id any more, as Kevin's suggestion. Address Kevin's comments: 2/8: remove device, id, name info in the error message, use error message in existing caller. A new function bdrv_snapshot_delete_by_id_or_name() is added to make the usage clear while keep logic unchanged. 3/8: remove device info in error message when name is empty. Use else if after call of bdrv_snapshot_find_by_id_and_name(). Other: 2/8: refined the comments in code for bdrv_snapshot_delete(). 3/8: in error reporting, change format from reason is: '%s' to reason is: %s. v7: rebased on upstream, target for 1.7. Wenchao Xia (8): 1 snapshot: new function bdrv_snapshot_find_by_id_and_name() 2 snapshot: distinguish id and name in snapshot delete 3 qmp: add internal snapshot support in qmp_transaction 4 qmp: add interface blockdev-snapshot-internal-sync 5 qmp: add interface blockdev-snapshot-delete-internal-sync 6 hmp: add interface hmp_snapshot_blkdev_internal 7 hmp: add interface hmp_snapshot_delete_blkdev_internal 8 qemu-iotests: add 057 internal snapshot for block device test case block/qcow2-snapshot.c | 55 +++--- block/qcow2.h |5 +- block/rbd.c| 21 - block/sheepdog.c |5 +- block/snapshot.c | 131 ++- blockdev.c | 190 hmp-commands.hx| 37 ++- hmp.c | 22 hmp.h |2 + include/block/block_int.h |5 +- include/block/snapshot.h | 14 +++- include/qemu-common.h |3 + qapi-schema.json | 66 +++- qemu-img.c | 11 ++- qmp-commands.hx| 104 -- savevm.c | 32 +++--- tests/qemu-iotests/057 | 259 tests/qemu-iotests/057.out |5 + tests/qemu-iotests/group |1 + 19 files changed, 914 insertions(+), 54 deletions(-) create mode 100755 tests/qemu-iotests/057 create mode 100644 tests/qemu-iotests/057.out Any comments for it? -- Best Regards Wenchao Xia
Re: [Qemu-devel] Are there plans to achieve ram live Snapshot feature?
于 2013-8-14 15:53, Stefan Hajnoczi 写道: On Wed, Aug 14, 2013 at 3:54 AM, Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-13 16:21, Stefan Hajnoczi 写道: On Tue, Aug 13, 2013 at 4:53 AM, Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-12 19:33, Stefan Hajnoczi 写道: On Mon, Aug 12, 2013 at 12:26 PM, Alex Bligh a...@alex.org.uk wrote: --On 12 August 2013 11:59:03 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: The idea that was discussed on qemu-devel@nongnu.org uses fork(2) to capture the state of guest RAM and then send it back to the parent process. The guest is only paused for a brief instant during fork(2) and can continue to run afterwards. How would you capture the state of emulated hardware which might not be in the guest RAM? Exactly the same way vmsave works today. It calls the device's save functions which serialize state to file. The difference between today's vmsave and the fork(2) approach is that QEMU does not need to wait for guest RAM to be written to file before resuming the guest. Stefan I have a worry about what glib says: On Unix, the GLib mainloop is incompatible with fork(). Any program using the mainloop must either exec() or exit() from the child without returning to the mainloop. This is fine, the child just writes out the memory pages and exits. It never returns to the glib mainloop. There is another way to do it: intercept the write in kvm.ko(or other kernel code). Since the key is intercept the memory change, we can do it in userspace in TCG mode, thus we can add the missing part in KVM mode. Another benefit of this way is: the used memory can be controlled. For example, with ioctl(), set a buffer of a fixed size which keeps the intercepted write data by kernel code, which can avoid frequently switch back to user space qemu code. when it is full always return back to userspace's qemu code, let qemu code save the data into disk. I haven't check the exactly behavior of Intel guest mode about how to handle page fault, so can't estimate the performance caused by switching of guest mode and root mode, but it should not be worse than fork(). The fork(2) approach is portable, covers both KVM and TCG, and doesn't require kernel changes. A kvm.ko kernel change also won't be supported on existing KVM hosts. These are big drawbacks and the kernel approach would need to be significantly better than plain old fork(2) to make it worthwhile. Stefan I think advantage is memory usage is predictable, so memory usage peak can be avoided, by always save the changed pages first. fork() does not know which pages are changed. I am not sure if this would be a serious issue when server's memory is consumed much, for example, 24G host emulate 11G*2 guest to provide powerful virtual server. Memory usage is predictable but guest uptime is unpredictable because it waits until memory is written out. This defeats the point of live savevm. The guest may be stalled arbitrarily. I think it is adjustable. There is no much difference with fork(), except get more precise control about the changed pages. Kernel intercept the change, and stores the changed page in another page, similar to fork(). When userspace qemu code execute, save some pages to disk. Buffer can be used like some lubricant. When Buffer = MAX, it equals to fork(), guest runs more lively. When Buffer = 0, guest runs less lively. I think it allows user to find a good balance point with a parameter. It is harder to implement, just want to show the idea. The fork child can minimize the chance of out-of-memory by using madvise(MADV_DONTNEED) after pages have been written out. It seems no way to make sure the written out page is the changed pages, so it have a good chance the written one is the unchanged and still used by the other qemu process. The way fork handles memory overcommit on Linux is configurable, but I guess in a situation where memory runs out the Out-of-Memory Killer will kill a process (probably QEMU since it is hogging so much memory). The risk of OOM can be avoided by running the traditional vmsave which stops the guest instead of using live vmsave. The other option is to live migrate to file but the disadvantage there is that you cannot choose exactly when the state it saved, it happens sometime after live migration is initiated. There are trade-offs with all the approaches, it depends on what is most important to you. Stefan -- Best Regards Wenchao Xia
Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes
于 2013-8-13 1:01, Michael Roth 写道: Quoting Paolo Bonzini (2013-08-12 02:30:28) 1) rename AioContext to AioSource. This is my major purpose, which declare it is not a context concept, and GMainContext is the entity represent the thread's activity. Note that the nested event loops in QEMU are _very_ different from glib nested event loops. In QEMU, nested event loops only run block layer events. In glib, they run all events. That's why you need AioContext. Would it be possible to use glib for our nested loops as well by just setting a higher priority for the AioContext GSource? Stefan and I were considering how we could make use of his drop ioflush patches to use a common mechanism to register fd events, but still allow us to distinguish between AioContext and non-AioContext for nested loops. I was originally thinking of using prepare() functions to filter out non-AioContext events, but that requires we implement on GSource's with that in mind, and non make use of pre-baked ones like GIOChannel's, and bakes block stuff into every event source implementation. Besides priority, also g_source_set_can_recurse() can help. With a deeper think, I found a harder problem: g_main_context_acquire() and g_main_context_release(). In release, pending BH/IO call back need to be cleared, but this action can't be triggered automatically when user call g_main_context_release(). For the above reason, I tend to think, maybe we should t wrap all of Glib's mainloop into custom encapsulation, such as QContext, Add the aio poll logic in q_context_release(). Use QContext * in every caller to hide GMainContext *, so QContext layer play the role of clear event loop API. Priorities didn't cross my mind though, but it seems pretty straightfoward... AioContext could then just be a container of sorts for managing bottom-halves and AioContext FDs and binding them to the proper GMainContext/MainLoop, but the underlying GSources could still be driven by a normal glib-based mainloop, just with a specific priority in the nested case. 2) Break AioSource into FdSource and BhSource. This make custom code less and simpler, one Gsource for one kind of job. It is not necessary but IMHO it will make things clear when add more things into main loop: add a new Gsource sub class, avoid to always have relationship with AioContext. But this is only complicating things work since users rely on both file- descriptor APIs and bottom half APIs. Taking things a step further, maybe AioContext can stop being a block-specific construct, but actually be the QContext we've discussed in the past for managing multiple event loops. All the block stuff would be hidden away in the GSource priority. For instance, #ifndef _WIN32 qemu_aio_set_fd_handler(fd, ...): aio_set_fd_handler(qemu_aio_context, fd, ..., QEMU_PRIORITY_BLOCK) qemu_set_fd_handler(fd, ...): aio_set_fd_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT) #else qemu_add_wait_object(fd, ...): add_wait_object(qemu_aio_context, fd, ...) qemu_set_fd_handler(fd, ...): set_socket_handler(qemu_aio_context, fd, ..., G_PRIORITY_DEFAULT) #endif qemu_bh_schedule: bh_schedule(qemu_aio_context, ...) etc... I'll be sending patches this week for moving add_wait_object/qemu_set_fd_handler to GSources, the non-global ones use GMainContext * to specify a non-default thread/context, but can be easily changed, or we can just do aioctx-g_main_context at the call sites. There's some nice possibilities in using the former though: avoiding O(n) lookups for stuff like finding the GSource for a particular event/event type, for instance, by storing pointers to the GSource or some kind of hashmap lookup. But probably better to discuss that aspect with some context so I'll try to get those patches out soon. More reasons: When I thinking how to bind library code to a thread context, it may need to add Context's concept into API of block.c. If I use AioContext, there will need a wrapper API to run the event loop. But If I got glib's GmainContext, things become simple. You already have it because AioContext is a GSource. You do not need to expose the AioContext, except as a GSource. I think expose GmainContext * or QContext *, is better than GSource *. int bdrv_read(GMainContext *ctx, BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors) Paolo -- Best Regards Wenchao Xia
Re: [Qemu-devel] Are there plans to achieve ram live Snapshot feature?
于 2013-8-13 16:21, Stefan Hajnoczi 写道: On Tue, Aug 13, 2013 at 4:53 AM, Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-12 19:33, Stefan Hajnoczi 写道: On Mon, Aug 12, 2013 at 12:26 PM, Alex Bligh a...@alex.org.uk wrote: --On 12 August 2013 11:59:03 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: The idea that was discussed on qemu-devel@nongnu.org uses fork(2) to capture the state of guest RAM and then send it back to the parent process. The guest is only paused for a brief instant during fork(2) and can continue to run afterwards. How would you capture the state of emulated hardware which might not be in the guest RAM? Exactly the same way vmsave works today. It calls the device's save functions which serialize state to file. The difference between today's vmsave and the fork(2) approach is that QEMU does not need to wait for guest RAM to be written to file before resuming the guest. Stefan I have a worry about what glib says: On Unix, the GLib mainloop is incompatible with fork(). Any program using the mainloop must either exec() or exit() from the child without returning to the mainloop. This is fine, the child just writes out the memory pages and exits. It never returns to the glib mainloop. There is another way to do it: intercept the write in kvm.ko(or other kernel code). Since the key is intercept the memory change, we can do it in userspace in TCG mode, thus we can add the missing part in KVM mode. Another benefit of this way is: the used memory can be controlled. For example, with ioctl(), set a buffer of a fixed size which keeps the intercepted write data by kernel code, which can avoid frequently switch back to user space qemu code. when it is full always return back to userspace's qemu code, let qemu code save the data into disk. I haven't check the exactly behavior of Intel guest mode about how to handle page fault, so can't estimate the performance caused by switching of guest mode and root mode, but it should not be worse than fork(). The fork(2) approach is portable, covers both KVM and TCG, and doesn't require kernel changes. A kvm.ko kernel change also won't be supported on existing KVM hosts. These are big drawbacks and the kernel approach would need to be significantly better than plain old fork(2) to make it worthwhile. Stefan I think advantage is memory usage is predictable, so memory usage peak can be avoided, by always save the changed pages first. fork() does not know which pages are changed. I am not sure if this would be a serious issue when server's memory is consumed much, for example, 24G host emulate 11G*2 guest to provide powerful virtual server. -- Best Regards Wenchao Xia
Re: [Qemu-devel] [RFC] Convert AioContext to Gsource sub classes
Il 10/08/2013 05:24, Wenchao Xia ha scritto:
Hi folks,
I'd like form a series which remove AioContext's concept and
bind to glib's main loop more closely. Since changed place will be
a bit much so want to know your opinion before real coding:
I'm not sure I understand... What does it buy you to split AioContext
this way? First, BhSource and FdSource are needed by block drivers, and
BhSource needs the notifier to interrupt the main loop. Second,
AioContext is _already_ a GSource exactly to integrate closely with
GLib's main loop. Look at the series that introduced AioContext back in
October 2012. The main AioContext is already added as a GSource to the
iothread's main loop; aio_wait is used in dataplane for simplicity, but
it could also use a separate GMainLoop and add AioContext there as a
GSource.
Paolo
It has two parts:
1) rename AioContext to AioSource.
This is my major purpose, which declare it is not a context concept,
and GMainContext is the entity represent the thread's activity. This
can prevent people add wrapper API such as g_main_context_acquire(),
g_main_context_prepare(See qcontext_prepare() in QContext patch). As a
result, standard glib's main loop API can be exposed, so I can add
GMainContext into block layer or any other API layer, instead of
custom encapsulation. For example:
int bdrv_read(GMainContext *ctx,
BlockDriverState *bs,
int64_t sector_num,
uint8_t *buf,
int nb_sectors)
In short, it avoid wrapper for GMainContext. I agree AioContext is
_already_ a GSource sub class now, there is no difficult to add
GMainContext *ctx for AioContext's reason. But I am afraid it becomes
not true with more patches comes, since the name is mis-guiding.
2) Break AioSource into FdSource and BhSource.
This make custom code less and simpler, one Gsource for one kind of
job. It is not necessary but IMHO it will make things clear when add
more things into main loop: add a new Gsource sub class, avoid to
always have relationship with AioContext.
changes:
**before patch:
typedef struct AioContext {
GSource source;
int walking_handlers;
QemuMutex bh_lock;
struct QEMUBH *first_bh;
int walking_bh;
EventNotifier notifier;
GArray *pollfds;
struct ThreadPool *thread_pool;
} AioContext;
**After patch:
typedef struct BhSource {
GSource source;
QemuMutex bh_lock;
struct QEMUBH *first_bh;
int walking_bh;
} BhSource;
typedef struct FdSource {
GSource source;
int walking_handlers;
EventNotifier notifier;
GArray *pollfds;
struct ThreadPool *thread_pool;
} FdSource;
Benefits:
Original code have a mix of Gsource and GMainContext's concept, we
may want to add wrapper functions around GMainContext's functions, such
as g_main_context_acquire(), g_main_context_prepare(), which brings
extra effort if you want to form a good and clear API layer. With
this changes, all qemu's custom code is attached under Gsource, we
have a clear GMainContext's API layer for event loop, no wrapper is
needed, and the event's loop API is glib's API, a clear layer let
me form a library or adding more things.
before:
qemu's mainloop caller, BH user, fd user
|
AioContext
|
GMainContext
after:
qemu's mainloop caller
| BH userfd user
GmainContext | |
||--BhSource |
|-FdSource
Note:
FdSource could be split more into ThreadSource and FdSource, which
distinguish more. It can be done easily if the change of this series
is online, when found necessary.
More reasons:
When I thinking how to bind library code to a thread context, it may
need to add Context's concept into API of block.c. If I use AioContext,
there will need a wrapper API to run the event loop. But If I got
glib's GmainContext, things become simple.
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
于 2013-8-10 19:05, Alex Bligh 写道: Paolo, On 9 Aug 2013, at 15:59, Paolo Bonzini wrote: It's not papering over anything. Timers right now are provided by the event loop. If you make AioContexts have timers, you can have a new AioContext for the timers that the event loop handles before your patches. It's not related to having two nested event loops. The nested event loops have the problem that timers do not run in them, but it's also a feature---because you know exactly what code runs in the nested event loop and what doesn't. Using an entirely distinct event loop preserves the feature. I've submitted a first cut as a separate patch just to see whether it works: http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg01412.html It passes 'make check' anyway. I didn't wrap TimerListGroup into a separate struct for reasons I'll set out below. I think there are two ways to go here: 1. Wrap TimerListGroup into a separate struct, leave all the TimerListGroup functions in. This probably makes it easier if (for instance) we decided to get rid of AioContexts entirely and make them g_source subclasses (per Wenchao Xia). I have a quick view of this series, but not very carefully since it is quite long.:) I have replied with Paolo's comments on my RFC thread. Personally I like g_source sub class, which make code clear, but let's discuss first if the direction is right in that mail. 2. Remove the TimerListGroup thing entirely and just have an array of TimerLists of size QEMU_CLOCK_MAX. I can leave the things that iterate that array inside qemu_timer.c (which I'd rather do for encapsulation). Part of the problem with the old timer code was that there were (in my opinion) far too many files that were working out what to iterate, and I really don't want to reintroduce that. Despite the fact we both dislike the name TimerListGroup, I think the way to go here is (1). (2) does not really save lines of code (certainly not compiled instructions) - it's main saving is removing a pile of commenting from include/qemu/timer.h, which makes things more opaque. I also think there may well be a use for something that wants to use timers but not AioContext (I am thinking for instance of a thread that does not do block IO). This permits that, but does not require it. WDYT? -- Best Regards Wenchao Xia
Re: [Qemu-devel] Are there plans to achieve ram live Snapshot feature?
于 2013-8-12 19:33, Stefan Hajnoczi 写道: On Mon, Aug 12, 2013 at 12:26 PM, Alex Bligh a...@alex.org.uk wrote: --On 12 August 2013 11:59:03 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: The idea that was discussed on qemu-devel@nongnu.org uses fork(2) to capture the state of guest RAM and then send it back to the parent process. The guest is only paused for a brief instant during fork(2) and can continue to run afterwards. How would you capture the state of emulated hardware which might not be in the guest RAM? Exactly the same way vmsave works today. It calls the device's save functions which serialize state to file. The difference between today's vmsave and the fork(2) approach is that QEMU does not need to wait for guest RAM to be written to file before resuming the guest. Stefan I have a worry about what glib says: On Unix, the GLib mainloop is incompatible with fork(). Any program using the mainloop must either exec() or exit() from the child without returning to the mainloop. There is another way to do it: intercept the write in kvm.ko(or other kernel code). Since the key is intercept the memory change, we can do it in userspace in TCG mode, thus we can add the missing part in KVM mode. Another benefit of this way is: the used memory can be controlled. For example, with ioctl(), set a buffer of a fixed size which keeps the intercepted write data by kernel code, which can avoid frequently switch back to user space qemu code. when it is full always return back to userspace's qemu code, let qemu code save the data into disk. I haven't check the exactly behavior of Intel guest mode about how to handle page fault, so can't estimate the performance caused by switching of guest mode and root mode, but it should not be worse than fork(). -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v6 00/18] aio: drop io_flush()
+ tests/test-thread-pool.c| 24 ++-- thread-pool.c | 11 +- 19 files changed, 163 insertions(+), 288 deletions(-) Ping? I tried to apply this series to do more work above it, but seems code conflict with upstream. -- Best Regards Wenchao Xia
[Qemu-devel] [RFC] Convert AioContext to Gsource sub classes
Hi folks,
I'd like form a series which remove AioContext's concept and
bind to glib's main loop more closely. Since changed place will be
a bit much so want to know your opinion before real coding:
changes:
**before patch:
typedef struct AioContext {
GSource source;
int walking_handlers;
QemuMutex bh_lock;
struct QEMUBH *first_bh;
int walking_bh;
EventNotifier notifier;
GArray *pollfds;
struct ThreadPool *thread_pool;
} AioContext;
**After patch:
typedef struct BhSource {
GSource source;
QemuMutex bh_lock;
struct QEMUBH *first_bh;
int walking_bh;
} BhSource;
typedef struct FdSource {
GSource source;
int walking_handlers;
EventNotifier notifier;
GArray *pollfds;
struct ThreadPool *thread_pool;
} FdSource;
Benefits:
Original code have a mix of Gsource and GMainContext's concept, we
may want to add wrapper functions around GMainContext's functions, such
as g_main_context_acquire(), g_main_context_prepare(), which brings
extra effort if you want to form a good and clear API layer. With
this changes, all qemu's custom code is attached under Gsource, we
have a clear GMainContext's API layer for event loop, no wrapper is
needed, and the event's loop API is glib's API, a clear layer let
me form a library or adding more things.
before:
qemu's mainloop caller, BH user, fd user
|
AioContext
|
GMainContext
after:
qemu's mainloop caller
| BH userfd user
GmainContext | |
||--BhSource |
|-FdSource
Note:
FdSource could be split more into ThreadSource and FdSource, which
distinguish more. It can be done easily if the change of this series
is online, when found necessary.
More reasons:
When I thinking how to bind library code to a thread context, it may
need to add Context's concept into API of block.c. If I use AioContext,
there will need a wrapper API to run the event loop. But If I got
glib's GmainContext, things become simple.
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 01/18] block: ensure bdrv_drain_all() works during bdrv_delete()
On Fri, Jul 26, 2013 at 02:43:44PM +0800, Wenchao Xia wrote: Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com One question: old code missed itself in bdrv_drain_all(), is that a bug? Sorry, I don't understand the question. Can you rephrase it? Before this patch, in the code path: bdrv_close()-bdrv_drain_all(), the *bs does not exist in bdrv_states, so the code missed the chance to drain the request on *bs. That is a bug, and this patch is actually a bugfix? In bdrv_delete() make sure to call bdrv_make_anon() *after* bdrv_close() so that the device is still seen by bdrv_drain_all() when iterating bdrv_states. Cc: qemu-sta...@nongnu.org Signed-off-by: Stefan Hajnoczi stefa...@redhat.com --- block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index 6cd39fa..9d68270 100644 --- a/block.c +++ b/block.c @@ -1600,11 +1600,11 @@ void bdrv_delete(BlockDriverState *bs) assert(!bs-job); assert(!bs-in_use); +bdrv_close(bs); + /* remove from list, if necessary */ bdrv_make_anon(bs); -bdrv_close(bs); - g_free(bs); } -- Best Regards Wenchao Xia -- Best Regards Wenchao Xia
[Qemu-devel] [PATCH V7 8/8] qemu-iotests: add 057 internal snapshot for block device test case
Create in transaction and deletion in single command will be tested.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
tests/qemu-iotests/057 | 259
tests/qemu-iotests/057.out |5 +
tests/qemu-iotests/group |1 +
3 files changed, 265 insertions(+), 0 deletions(-)
create mode 100755 tests/qemu-iotests/057
create mode 100644 tests/qemu-iotests/057.out
diff --git a/tests/qemu-iotests/057 b/tests/qemu-iotests/057
new file mode 100755
index 000..9cdd582
--- /dev/null
+++ b/tests/qemu-iotests/057
@@ -0,0 +1,259 @@
+#!/usr/bin/env python
+#
+# Tests for internal snapshot.
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# Based on 055.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see http://www.gnu.org/licenses/.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_drv_base_name = 'drive'
+
+class ImageSnapshotTestCase(iotests.QMPTestCase):
+image_len = 120 * 1024 * 1024 # MB
+
+def __init__(self, *args):
+self.expect = []
+super(ImageSnapshotTestCase, self).__init__(*args)
+
+def _setUp(self, test_img_base_name, image_num):
+self.vm = iotests.VM()
+for i in range(0, image_num):
+filename = '%s%d' % (test_img_base_name, i)
+img = os.path.join(iotests.test_dir, filename)
+device = '%s%d' % (test_drv_base_name, i)
+qemu_img('create', '-f', iotests.imgfmt, img, str(self.image_len))
+self.vm.add_drive(img)
+self.expect.append({'image': img, 'device': device,
+'snapshots': [],
+'snapshots_name_counter': 0})
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+for dev_expect in self.expect:
+os.remove(dev_expect['image'])
+
+def createSnapshotInTransaction(self, snapshot_num, abort = False):
+actions = []
+for dev_expect in self.expect:
+num = dev_expect['snapshots_name_counter']
+for j in range(0, snapshot_num):
+name = '%s_sn%d' % (dev_expect['device'], num)
+num = num + 1
+if abort == False:
+dev_expect['snapshots'].append({'name': name})
+dev_expect['snapshots_name_counter'] = num
+actions.append({
+'type': 'blockdev-snapshot-internal-sync',
+'data': { 'device': dev_expect['device'],
+ 'name': name },
+})
+
+if abort == True:
+actions.append({
+'type': 'abort',
+'data': {},
+})
+
+result = self.vm.qmp('transaction', actions = actions)
+
+if abort == True:
+self.assert_qmp(result, 'error/class', 'GenericError')
+else:
+self.assert_qmp(result, 'return', {})
+
+def verifySnapshotInfo(self):
+result = self.vm.qmp('query-block')
+
+# Verify each expected result
+for dev_expect in self.expect:
+# 1. Find the returned image value and snapshot info
+image_result = None
+for device in result['return']:
+if device['device'] == dev_expect['device']:
+image_result = device['inserted']['image']
+break
+self.assertTrue(image_result != None)
+# Do not consider zero snapshot case now
+sn_list_result = image_result['snapshots']
+sn_list_expect = dev_expect['snapshots']
+
+# 2. Verify it with expect
+self.assertTrue(len(sn_list_result) == len(sn_list_expect))
+
+for sn_expect in sn_list_expect:
+sn_result = None
+for sn in sn_list_result:
+if sn_expect['name'] == sn['name']:
+sn_result = sn
+break
+self.assertTrue(sn_result != None)
+# Fill in the detail info
+sn_expect.update(sn_result)
+
+def deleteSnapshot(self, device, id = None, name = None):
+sn_list_expect = None
+sn_expect = None
+
+self.assertTrue(id != None or name != None)
+
+# Fill in the detail info include ID
+self.verifySnapshotInfo()
+
+#find
[Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete
Snapshot creation actually already distinguish id and name since it take
a structured parameter *sn, but delete can't. Later an accurate delete
is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
so change its prototype. Also *errp is added to tip error, but return
value is kepted to let caller check what kind of error happens. Existing
caller for it are savevm, delvm and qemu-img, they are not impacted by
introducing a new function bdrv_snapshot_delete_by_id_or_name(), which
check the return value and do the operation again.
Before this patch:
For qcow2, it search id first then name to find the one to delete.
For rbd, it search name.
For sheepdog, it does nothing.
After this patch:
For qcow2, logic is the same by call it twice in caller.
For rbd, it always fails in delete with id, but still search for name
in second try, no change to user.
Some code for *errp is based on Pavel's patch.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
block/qcow2-snapshot.c| 55 ++
block/qcow2.h |5 +++-
block/rbd.c | 21 +++-
block/sheepdog.c |5 +++-
block/snapshot.c | 58 ++--
include/block/block_int.h |5 +++-
include/block/snapshot.h |8 +-
qemu-img.c| 11 +---
savevm.c | 32 +---
9 files changed, 157 insertions(+), 43 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0caac90..6a0ed6b 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -288,31 +288,47 @@ static void find_new_snapshot_id(BlockDriverState *bs,
snprintf(id_str, id_str_size, %d, id_max + 1);
}
-static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
+static int find_snapshot_by_id_and_name(BlockDriverState *bs,
+const char *id,
+const char *name)
{
BDRVQcowState *s = bs-opaque;
int i;
-for(i = 0; i s-nb_snapshots; i++) {
-if (!strcmp(s-snapshots[i].id_str, id_str))
-return i;
+if (id name) {
+for (i = 0; i s-nb_snapshots; i++) {
+if (!strcmp(s-snapshots[i].id_str, id)
+!strcmp(s-snapshots[i].name, name)) {
+return i;
+}
+}
+} else if (id) {
+for (i = 0; i s-nb_snapshots; i++) {
+if (!strcmp(s-snapshots[i].id_str, id)) {
+return i;
+}
+}
+} else if (name) {
+for (i = 0; i s-nb_snapshots; i++) {
+if (!strcmp(s-snapshots[i].name, name)) {
+return i;
+}
+}
}
+
return -1;
}
-static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
+static int find_snapshot_by_id_or_name(BlockDriverState *bs,
+ const char *id_or_name)
{
-BDRVQcowState *s = bs-opaque;
-int i, ret;
+int ret;
-ret = find_snapshot_by_id(bs, name);
-if (ret = 0)
+ret = find_snapshot_by_id_and_name(bs, id_or_name, NULL);
+if (ret = 0) {
return ret;
-for(i = 0; i s-nb_snapshots; i++) {
-if (!strcmp(s-snapshots[i].name, name))
-return i;
}
-return -1;
+return find_snapshot_by_id_and_name(bs, NULL, id_or_name);
}
/* if no id is provided, a new one is constructed */
@@ -334,7 +350,7 @@ int qcow2_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)
}
/* Check that the ID is unique */
-if (find_snapshot_by_id(bs, sn_info-id_str) = 0) {
+if (find_snapshot_by_id_and_name(bs, sn_info-id_str, NULL) = 0) {
return -EEXIST;
}
@@ -531,15 +547,19 @@ fail:
return ret;
}
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp)
{
BDRVQcowState *s = bs-opaque;
QCowSnapshot sn;
int snapshot_index, ret;
/* Search the snapshot */
-snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
+snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
if (snapshot_index 0) {
+error_setg(errp, Can't find the snapshot);
return -ENOENT;
}
sn = s-snapshots[snapshot_index];
@@ -551,6 +571,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char
*snapshot_id)
s-nb_snapshots--;
ret = qcow2_write_snapshots(bs);
if (ret 0) {
+error_setg(errp, Failed to remove snapshot from snapshot list);
return ret;
}
@@ -568,6 +589,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char
*snapshot_id)
ret
Re: [Qemu-devel] [PATCH V6 0/8] add internal snapshot support at block device level
于 2013-7-30 10:59, Wenchao Xia 写道: This series brings internal snapshot support at block devices level, now we have two three methods to do block snapshot lively: 1) backing chain, 2) internal one and 3) drive-back up approach. Comparation: Advantages:Disadvantages: 1)delta data, taken fast, export, sizeperformance, delete slow. 2) taken fast, delete fast, performance, size delta data, format 3) performance, export, format taken slow, delta data, size I think in most case, saving vmstate in an standalone file is better than saving it inside qcow2, So suggest treat internal snapshot as block level methods and not encourage user to savevm in qcow2 any more. Implemention details: To avoid trouble, this serial have hide ID in create interfaces, this make sure no chaos of ID and name will be introduced by these interfaces. There is one patch may be common to Pavel's savvm transaction, patch 1/11, others are not quite related. Patch 1/11 will not set errp when no snapshot find, since patch 3/11 need to distinguish real error case. Next steps to better full VM snapshot: Improve internal snapshot's export capability. Better vmstate saving. Thanks Kevin to give advisement about how add it in qmp_transaction, oldest version comes drom Dietmar Maurer. v3: General: Rebased after Stenfan's driver-backup patch V6. Address Eric's comments: 4/9: grammar fix and better doc. 5/9: parameter name is mandatory now. grammar fix. 6/9: redesiged interface: take both id and name as optional parameter, return the deleted snapshot's info. Address Stefan's comments: 4/9: add '' around %s in message. drop code comments about vm_clock. 9/9: better doc, refined the code and add more test case. v4: Address Stefan's comments: 4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(), spell fix and better doc. 5/9: better doc. 6/9: remove spurious ';' in code, spell fix and better doc. v5: Address Kevin's comments: 3/8, 4/8, 8/8: remove the limit of numeric snapshot name. General change: 4/8: use existing type as parameter in qapi schema. v6: Address Stefan's comments: 2/8: macro STR_PRINT_CHAR was renamed as STR_OR_NULL, and moved into patch 5, since implement function in this patch do not printf snapshot id any more, as Kevin's suggestion. Address Kevin's comments: 2/8: remove device, id, name info in the error message, use error message in existing caller. A new function bdrv_snapshot_delete_by_id_or_name() is added to make the usage clear while keep logic unchanged. 3/8: remove device info in error message when name is empty. Use else if after call of bdrv_snapshot_find_by_id_and_name(). Other: 2/8: refined the comments in code for bdrv_snapshot_delete(). 3/8: in error reporting, change format from reason is: '%s' to reason is: %s. Wenchao Xia (8): 1 snapshot: new function bdrv_snapshot_find_by_id_and_name() 2 snapshot: distinguish id and name in snapshot delete 3 qmp: add internal snapshot support in qmp_transaction 4 qmp: add interface blockdev-snapshot-internal-sync 5 qmp: add interface blockdev-snapshot-delete-internal-sync 6 hmp: add interface hmp_snapshot_blkdev_internal 7 hmp: add interface hmp_snapshot_delete_blkdev_internal 8 qemu-iotests: add 056 internal snapshot for block device test case block/qcow2-snapshot.c | 55 +++--- block/qcow2.h |5 +- block/rbd.c| 21 - block/sheepdog.c |5 +- block/snapshot.c | 131 ++- blockdev.c | 190 hmp-commands.hx| 37 ++- hmp.c | 22 hmp.h |2 + include/block/block_int.h |5 +- include/block/snapshot.h | 14 +++- include/qemu-common.h |3 + qapi-schema.json | 65 +++- qemu-img.c | 11 ++- qmp-commands.hx| 104 -- savevm.c | 32 +++--- tests/qemu-iotests/056 | 259 tests/qemu-iotests/056.out |5 + tests/qemu-iotests/group |1 + 19 files changed, 913 insertions(+), 54 deletions(-) create mode 100755 tests/qemu-iotests/056 create mode 100644 tests/qemu-iotests/056.out Hello, any comments for this version? I just found 056 is taken in upstream this morning, may respin if you like. I guess it is hard to catch 1.6, so rebased on upstream and re-target to 1.7. -- Best Regards Wenchao Xia
[Qemu-devel] [PATCH V7 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync
This interface use id and name as optional parameters, to handle the
case that one image contain multiple snapshots with same name which
may be '', but with different id.
Adding parameter id is for historical compatiability reason, and
that case is not possible in qemu's new interface for internal
snapshot at block device level, but still possible in qemu-img.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
blockdev.c| 61 +
include/qemu-common.h |3 ++
qapi-schema.json | 27 +
qmp-commands.hx | 41 +
4 files changed, 132 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b4754ef..13de395 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -869,6 +869,67 @@ void qmp_blockdev_snapshot_internal_sync(const char
*device,
snapshot, errp);
}
+SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
+ bool has_id,
+ const char *id,
+ bool has_name,
+ const char *name,
+ Error **errp)
+{
+BlockDriverState *bs = bdrv_find(device);
+QEMUSnapshotInfo sn;
+Error *local_err = NULL;
+SnapshotInfo *info = NULL;
+int ret;
+
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return NULL;
+}
+
+if (!has_id) {
+id = NULL;
+}
+
+if (!has_name) {
+name = NULL;
+}
+
+if (!id !name) {
+error_setg(errp, Name or id must be provided);
+return NULL;
+}
+
+ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, sn, local_err);
+if (error_is_set(local_err)) {
+error_propagate(errp, local_err);
+return NULL;
+}
+if (!ret) {
+error_setg(errp,
+ Snapshot with id '%s' and name '%s' does not exist on
+ device '%s',
+ STR_OR_NULL(id), STR_OR_NULL(name), device);
+return NULL;
+}
+
+bdrv_snapshot_delete(bs, id, name, local_err);
+if (error_is_set(local_err)) {
+error_propagate(errp, local_err);
+return NULL;
+}
+
+info = g_malloc0(sizeof(SnapshotInfo));
+info-id = g_strdup(sn.id_str);
+info-name = g_strdup(sn.name);
+info-date_nsec = sn.date_nsec;
+info-date_sec = sn.date_sec;
+info-vm_state_size = sn.vm_state_size;
+info-vm_clock_nsec = sn.vm_clock_nsec % 10;
+info-vm_clock_sec = sn.vm_clock_nsec / 10;
+
+return info;
+}
/* New and old BlockDriverState structs for group snapshots */
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 6948bb9..5054836 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const
char default_suffix);
int64_t strtosz_suffix_unit(const char *nptr, char **end,
const char default_suffix, int64_t unit);
+/* used to print char* safely */
+#define STR_OR_NULL(str) ((str) ? (str) : null)
+
/* path.c */
void init_paths(const char *prefix);
const char *path(const char *pathname);
diff --git a/qapi-schema.json b/qapi-schema.json
index a236cde..5a46abe 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1778,6 +1778,33 @@
'data': 'BlockdevSnapshotInternal' }
##
+# @blockdev-snapshot-delete-internal-sync
+#
+# Synchronously delete an internal snapshot of a block device, when the format
+# of the image used support it. The snapshot is identified by name or id or
+# both. One of the name or id is required. Return SnapshotInfo for the
+# successfully deleted snapshot.
+#
+# @device: the name of the device to delete the snapshot from
+#
+# @id: optional the snapshot's ID to be deleted
+#
+# @name: optional the snapshot's name to be deleted
+#
+# Returns: SnapshotInfo on success
+# If @device is not a valid block device, DeviceNotFound
+# If snapshot not found, GenericError
+# If the format of the image used does not support it,
+# BlockFormatFeatureNotSupported
+# If @id and @name are both not specified, GenericError
+#
+# Since 1.7
+##
+{ 'command': 'blockdev-snapshot-delete-internal-sync',
+ 'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
+ 'returns': 'SnapshotInfo' }
+
+##
# @human-monitor-command:
#
# Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6097283..3d34a1c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1127,6 +1127,47 @@ Example:
EQMP
{
+.name = blockdev-snapshot-delete-internal-sync,
+.args_type = device:B,id:s?,name:s
[Qemu-devel] [PATCH V7 3/8] qmp: add internal snapshot support in qmp_transaction
Unlike savevm, the qmp_transaction interface will not generate
snapshot name automatically, saving trouble to return information
of the new created snapshot.
Although qcow2 support storing multiple snapshots with same name
but different ID, here it will fail when an snapshot with that name
already exist before the operation. Format such as rbd do not support
ID at all, and in most case, it means trouble to user when he faces
multiple snapshots with same name, so ban that case. Request with
empty name will be rejected.
Snapshot ID can't be specified in this interface.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
blockdev.c | 116 ++
qapi-schema.json | 19 -
qmp-commands.hx | 34
3 files changed, 160 insertions(+), 9 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 41b0a49..0d717bc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -887,6 +887,117 @@ struct BlkTransactionState {
QSIMPLEQ_ENTRY(BlkTransactionState) entry;
};
+/* internal snapshot private data */
+typedef struct InternalSnapshotState {
+BlkTransactionState common;
+BlockDriverState *bs;
+QEMUSnapshotInfo sn;
+} InternalSnapshotState;
+
+static void internal_snapshot_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+const char *device;
+const char *name;
+BlockDriverState *bs;
+QEMUSnapshotInfo old_sn, *sn;
+bool ret;
+qemu_timeval tv;
+BlockdevSnapshotInternal *internal;
+InternalSnapshotState *state;
+int ret1;
+
+g_assert(common-action-kind ==
+ TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
+internal = common-action-blockdev_snapshot_internal_sync;
+state = DO_UPCAST(InternalSnapshotState, common, common);
+
+/* 1. parse input */
+device = internal-device;
+name = internal-name;
+
+/* 2. check for validation */
+bs = bdrv_find(device);
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return;
+}
+
+if (!bdrv_is_inserted(bs)) {
+error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+return;
+}
+
+if (bdrv_is_read_only(bs)) {
+error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+return;
+}
+
+if (!bdrv_can_snapshot(bs)) {
+error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+ bs-drv-format_name, device, internal snapshot);
+return;
+}
+
+if (!strlen(name)) {
+error_setg(errp, Name is empty);
+return;
+}
+
+/* check whether a snapshot with name exist */
+ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, old_sn, errp);
+if (error_is_set(errp)) {
+return;
+} else if (ret) {
+error_setg(errp,
+ Snapshot with name '%s' already exists on device '%s',
+ name, device);
+return;
+}
+
+/* 3. take the snapshot */
+sn = state-sn;
+pstrcpy(sn-name, sizeof(sn-name), name);
+qemu_gettimeofday(tv);
+sn-date_sec = tv.tv_sec;
+sn-date_nsec = tv.tv_usec * 1000;
+sn-vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+
+ret1 = bdrv_snapshot_create(bs, sn);
+if (ret1 0) {
+error_setg_errno(errp, -ret1,
+ Failed to create snapshot '%s' on device '%s',
+ name, device);
+return;
+}
+
+/* 4. succeed, mark a snapshot is created */
+state-bs = bs;
+}
+
+static void internal_snapshot_abort(BlkTransactionState *common)
+{
+InternalSnapshotState *state =
+ DO_UPCAST(InternalSnapshotState, common, common);
+BlockDriverState *bs = state-bs;
+QEMUSnapshotInfo *sn = state-sn;
+Error *local_error = NULL;
+
+if (!bs) {
+return;
+}
+
+if (bdrv_snapshot_delete(bs, sn-id_str, sn-name, local_error) 0) {
+error_report(Failed to delete snapshot with id '%s' and name '%s' on
+ device '%s' in abort, reason is: %s,
+ sn-id_str,
+ sn-name,
+ bdrv_get_device_name(bs),
+ error_get_pretty(local_error));
+error_free(local_error);
+}
+}
+
/* external snapshot private data */
typedef struct ExternalSnapshotState {
BlkTransactionState common;
@@ -1070,6 +1181,11 @@ static const BdrvActionOps actions[] = {
.prepare = abort_prepare,
.commit = abort_commit,
},
+[TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
+.instance_size = sizeof(InternalSnapshotState),
+.prepare = internal_snapshot_prepare,
+.abort = internal_snapshot_abort,
+},
};
/*
diff --git a/qapi-schema.json b/qapi-schema.json
index a51f7d2..38663c5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1639,6 +1639,22 @@
'*mode': 'NewImageMode
[Qemu-devel] [PATCH V7 6/8] hmp: add interface hmp_snapshot_blkdev_internal
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
hmp-commands.hx | 19 +--
hmp.c | 10 ++
hmp.h |1 +
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8c6b91a..039c401 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1023,8 +1023,7 @@ ETEXI
of device. If a new image file is specified,
the\n\t\t\t
new image file will become the new root image.\n\t\t\t
If format is specified, the snapshot file will\n\t\t\t
- be created in that format. Otherwise the\n\t\t\t
- snapshot will be internal! (currently
unsupported).\n\t\t\t
+ be created in that format.\n\t\t\t
The default format is qcow2. The -n flag requests
QEMU\n\t\t\t
to reuse the image found in new-image-file, instead
of\n\t\t\t
recreating it from scratch.,
@@ -1038,6 +1037,22 @@ Snapshot device, using snapshot file as target if
provided
ETEXI
{
+.name = snapshot_blkdev_internal,
+.args_type = device:B,name:s,
+.params = device name,
+.help = take an internal snapshot of device.\n\t\t\t
+ The format of the image used by device must\n\t\t\t
+ support it, such as qcow2.\n\t\t\t,
+.mhandler.cmd = hmp_snapshot_blkdev_internal,
+},
+
+STEXI
+@item snapshot_blkdev_internal
+@findex snapshot_blkdev_internal
+Take an internal snapshot on device if it support
+ETEXI
+
+{
.name = drive_mirror,
.args_type = reuse:-n,full:-f,device:B,target:s,format:s?,
.params = [-n] [-f] device target [format],
diff --git a/hmp.c b/hmp.c
index c45514b..34763be 100644
--- a/hmp.c
+++ b/hmp.c
@@ -962,6 +962,16 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, errp);
}
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, device);
+const char *name = qdict_get_str(qdict, name);
+Error *errp = NULL;
+
+qmp_blockdev_snapshot_internal_sync(device, name, errp);
+hmp_handle_error(mon, errp);
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 6c3bdcd..cc1ca58 100644
--- a/hmp.h
+++ b/hmp.h
@@ -54,6 +54,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict);
void hmp_balloon(Monitor *mon, const QDict *qdict);
void hmp_block_resize(Monitor *mon, const QDict *qdict);
void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
void hmp_drive_backup(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
--
1.7.1
[Qemu-devel] [PATCH V7 4/8] qmp: add interface blockdev-snapshot-internal-sync
Snapshot ID can't be specified in this interface.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
blockdev.c | 13 +
qapi-schema.json | 20
qmp-commands.hx | 29 +
3 files changed, 62 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 0d717bc..b4754ef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -856,6 +856,19 @@ void qmp_blockdev_snapshot_sync(const char *device, const
char *snapshot_file,
snapshot, errp);
}
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+ const char *name,
+ Error **errp)
+{
+BlockdevSnapshotInternal snapshot = {
+.device = (char *) device,
+.name = (char *) name
+};
+
+blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+ snapshot, errp);
+}
+
/* New and old BlockDriverState structs for group snapshots */
diff --git a/qapi-schema.json b/qapi-schema.json
index 38663c5..a236cde 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1758,6 +1758,26 @@
'data': 'BlockdevSnapshot' }
##
+# @blockdev-snapshot-internal-sync
+#
+# Synchronously take an internal snapshot of a block device, when the format
+# of the image used supports it.
+#
+# For the arguments, see the documentation of BlockdevSnapshotInternal.
+#
+# Returns: nothing on success
+# If @device is not a valid block device, DeviceNotFound
+# If any snapshot matching @name exists, or @name is empty,
+# GenericError
+# If the format of the image used does not support it,
+# BlockFormatFeatureNotSupported
+#
+# Since 1.7
+##
+{ 'command': 'blockdev-snapshot-internal-sync',
+ 'data': 'BlockdevSnapshotInternal' }
+
+##
# @human-monitor-command:
#
# Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 68a1611..6097283 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1098,6 +1098,35 @@ Example:
EQMP
{
+.name = blockdev-snapshot-internal-sync,
+.args_type = device:B,name:s,
+.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
+},
+
+SQMP
+blockdev-snapshot-internal-sync
+---
+
+Synchronously take an internal snapshot of a block device when the format of
+image used supports it. If the name is an empty string, or a snapshot with
+name already exists, the operation will fail.
+
+Arguments:
+
+- device: device name to snapshot (json-string)
+- name: name of the new snapshot (json-string)
+
+Example:
+
+- { execute: blockdev-snapshot-internal-sync,
+arguments: { device: ide-hd0,
+ name: snapshot0 }
+ }
+- { return: {} }
+
+EQMP
+
+{
.name = drive-mirror,
.args_type = sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,
on-source-error:s?,on-target-error:s?,
--
1.7.1
[Qemu-devel] [PATCH V7 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal
It is hard to make both id and name optional in hmp console as qmp
interface, so this interface require user to specify name.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
hmp-commands.hx | 18 ++
hmp.c | 12
hmp.h |1 +
3 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 039c401..96b1874 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1053,6 +1053,24 @@ Take an internal snapshot on device if it support
ETEXI
{
+.name = snapshot_delete_blkdev_internal,
+.args_type = device:B,name:s,id:s?,
+.params = device name [id],
+.help = delete an internal snapshot of device.\n\t\t\t
+ If id is specified, qemu will try delete\n\t\t\t
+ the snapshot matching both id and name.\n\t\t\t
+ The format of the image used by device must\n\t\t\t
+ support it, such as qcow2.\n\t\t\t,
+.mhandler.cmd = hmp_snapshot_delete_blkdev_internal,
+},
+
+STEXI
+@item snapshot_delete_blkdev_internal
+@findex snapshot_delete_blkdev_internal
+Delete an internal snapshot on device if it support
+ETEXI
+
+{
.name = drive_mirror,
.args_type = reuse:-n,full:-f,device:B,target:s,format:s?,
.params = [-n] [-f] device target [format],
diff --git a/hmp.c b/hmp.c
index 34763be..e2139d0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -972,6 +972,18 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const
QDict *qdict)
hmp_handle_error(mon, errp);
}
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, device);
+const char *name = qdict_get_str(qdict, name);
+const char *id = qdict_get_try_str(qdict, id);
+Error *errp = NULL;
+
+qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
+ true, name, errp);
+hmp_handle_error(mon, errp);
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index cc1ca58..54cf71f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -55,6 +55,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict);
void hmp_block_resize(Monitor *mon, const QDict *qdict);
void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
void hmp_drive_backup(Monitor *mon, const QDict *qdict);
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
--
1.7.1
[Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
This series brings internal snapshot support at block devices level, now we have two three methods to do block snapshot lively: 1) backing chain, 2) internal one and 3) drive-back up approach. Comparation: Advantages:Disadvantages: 1)delta data, taken fast, export, sizeperformance, delete slow. 2) taken fast, delete fast, performance, size delta data, format 3) performance, export, format taken slow, delta data, size, host I/O I think in most case, saving vmstate in an standalone file is better than saving it inside qcow2, So suggest treat internal snapshot as block level methods and not encourage user to savevm in qcow2 any more. Implemention details: To avoid trouble, this serial have hide ID in create interfaces, this make sure no chaos of ID and name will be introduced by these interfaces. There is one patch may be common to Pavel's savvm transaction, patch 1/11, others are not quite related. Patch 1/11 will not set errp when no snapshot find, since patch 3/11 need to distinguish real error case. Next steps to better full VM snapshot: Improve internal snapshot's export capability. Better vmstate saving. Thanks Kevin to give advisement about how add it in qmp_transaction, oldest version comes drom Dietmar Maurer. v3: General: Rebased after Stenfan's driver-backup patch V6. Address Eric's comments: 4/9: grammar fix and better doc. 5/9: parameter name is mandatory now. grammar fix. 6/9: redesiged interface: take both id and name as optional parameter, return the deleted snapshot's info. Address Stefan's comments: 4/9: add '' around %s in message. drop code comments about vm_clock. 9/9: better doc, refined the code and add more test case. v4: Address Stefan's comments: 4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(), spell fix and better doc. 5/9: better doc. 6/9: remove spurious ';' in code, spell fix and better doc. v5: Address Kevin's comments: 3/8, 4/8, 8/8: remove the limit of numeric snapshot name. General change: 4/8: use existing type as parameter in qapi schema. v6: Address Stefan's comments: 2/8: macro STR_PRINT_CHAR was renamed as STR_OR_NULL, and moved into patch 5, since implement function in this patch do not printf snapshot id any more, as Kevin's suggestion. Address Kevin's comments: 2/8: remove device, id, name info in the error message, use error message in existing caller. A new function bdrv_snapshot_delete_by_id_or_name() is added to make the usage clear while keep logic unchanged. 3/8: remove device info in error message when name is empty. Use else if after call of bdrv_snapshot_find_by_id_and_name(). Other: 2/8: refined the comments in code for bdrv_snapshot_delete(). 3/8: in error reporting, change format from reason is: '%s' to reason is: %s. v7: rebased on upstream, target for 1.7. Wenchao Xia (8): 1 snapshot: new function bdrv_snapshot_find_by_id_and_name() 2 snapshot: distinguish id and name in snapshot delete 3 qmp: add internal snapshot support in qmp_transaction 4 qmp: add interface blockdev-snapshot-internal-sync 5 qmp: add interface blockdev-snapshot-delete-internal-sync 6 hmp: add interface hmp_snapshot_blkdev_internal 7 hmp: add interface hmp_snapshot_delete_blkdev_internal 8 qemu-iotests: add 057 internal snapshot for block device test case block/qcow2-snapshot.c | 55 +++--- block/qcow2.h |5 +- block/rbd.c| 21 - block/sheepdog.c |5 +- block/snapshot.c | 131 ++- blockdev.c | 190 hmp-commands.hx| 37 ++- hmp.c | 22 hmp.h |2 + include/block/block_int.h |5 +- include/block/snapshot.h | 14 +++- include/qemu-common.h |3 + qapi-schema.json | 66 +++- qemu-img.c | 11 ++- qmp-commands.hx| 104 -- savevm.c | 32 +++--- tests/qemu-iotests/057 | 259 tests/qemu-iotests/057.out |5 + tests/qemu-iotests/group |1 + 19 files changed, 914 insertions(+), 54 deletions(-) create mode 100755 tests/qemu-iotests/057 create mode 100644 tests/qemu-iotests/057.out
[Qemu-devel] [PATCH V7 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name()
To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.
Some code are modified based on Pavel's patch.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
block/snapshot.c | 73 ++
include/block/snapshot.h |6
2 files changed, 79 insertions(+), 0 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..481a3ee 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,79 @@ int bdrv_snapshot_find(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info,
return ret;
}
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
+{
+QEMUSnapshotInfo *sn_tab, *sn;
+int nb_sns, i;
+bool ret = false;
+
+assert(id || name);
+
+nb_sns = bdrv_snapshot_list(bs, sn_tab);
+if (nb_sns 0) {
+error_setg_errno(errp, -nb_sns, Failed to get a snapshot list);
+return false;
+} else if (nb_sns == 0) {
+return false;
+}
+
+if (id name) {
+for (i = 0; i nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id) !strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (id) {
+for (i = 0; i nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-id_str, id)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+} else if (name) {
+for (i = 0; i nb_sns; i++) {
+sn = sn_tab[i];
+if (!strcmp(sn-name, name)) {
+*sn_info = *sn;
+ret = true;
+break;
+}
+}
+}
+
+g_free(sn_tab);
+return ret;
+}
+
int bdrv_can_snapshot(BlockDriverState *bs)
{
BlockDriver *drv = bs-drv;
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index eaf61f0..9d06dc1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,6 +26,7 @@
#define SNAPSHOT_H
#include qemu-common.h
+#include qapi/error.h
typedef struct QEMUSnapshotInfo {
char id_str[128]; /* unique snapshot id */
@@ -40,6 +41,11 @@ typedef struct QEMUSnapshotInfo {
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
const char *name);
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
--
1.7.1
Re: [Qemu-devel] [PATCH v3 1/7] vvfat: use bdrv_new() to allocate BlockDriverState
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com we need bdrv_new() to properly initialize BDS, don't allocate memory manually. Signed-off-by: Fam Zheng f...@redhat.com --- block/vvfat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/vvfat.c b/block/vvfat.c index cd3b8ed..a827d91 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2943,7 +2943,7 @@ static int enable_write_target(BDRVVVFATState *s) unlink(s-qcow_filename); #endif -s-bs-backing_hd = calloc(sizeof(BlockDriverState), 1); +s-bs-backing_hd = bdrv_new(); s-bs-backing_hd-drv = vvfat_write_target; s-bs-backing_hd-opaque = g_malloc(sizeof(void*)); *(void**)s-bs-backing_hd-opaque = s; -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v3 2/7] iscsi: use bdrv_new() instead of stack structure
于 2013-7-31 18:13, Fam Zheng 写道:
BlockDriverState structure needs bdrv_new() to initialize refcnt, don't
allocate a local structure variable and memset to 0, becasue with coming
refcnt implementation, bdrv_unref will crash if bs-refcnt not
initialized to 1.
Signed-off-by: Fam Zheng f...@redhat.com
---
block/iscsi.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 5f28c6a..db8a699 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1247,11 +1247,11 @@ static int iscsi_create(const char *filename,
QEMUOptionParameter *options)
{
int ret = 0;
int64_t total_size = 0;
-BlockDriverState bs;
+BlockDriverState *bs;
IscsiLun *iscsilun = NULL;
QDict *bs_options;
-memset(bs, 0, sizeof(BlockDriverState));
+bs = bdrv_new();
/* Read out options */
while (options options-name) {
@@ -1261,12 +1261,12 @@ static int iscsi_create(const char *filename,
QEMUOptionParameter *options)
options++;
}
-bs.opaque = g_malloc0(sizeof(struct IscsiLun));
-iscsilun = bs.opaque;
+bs-opaque = g_malloc0(sizeof(struct IscsiLun));
+iscsilun = bs-opaque;
bs_options = qdict_new();
qdict_put(bs_options, filename, qstring_from_str(filename));
-ret = iscsi_open(bs, bs_options, 0);
+ret = iscsi_open(bs, bs_options, 0);
QDECREF(bs_options);
if (ret != 0) {
@@ -1280,7 +1280,7 @@ static int iscsi_create(const char *filename,
QEMUOptionParameter *options)
ret = -ENODEV;
goto out;
}
-if (bs.total_sectors total_size) {
+if (bs-total_sectors total_size) {
ret = -ENOSPC;
goto out;
}
@@ -1290,7 +1290,7 @@ out:
if (iscsilun-iscsi != NULL) {
iscsi_destroy_context(iscsilun-iscsi);
}
-g_free(bs.opaque);
bs,opaque seems leaked. bdrv_delete() will not free it unless bs-drv
!= NULL.
+bdrv_delete(bs);
return ret;
}
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v3 3/7] block: implement reference count for BlockDriverState
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com Introduce bdrv_ref/bdrv_unref to manage the lifecycle of BlockDriverState. They are unused for now but will used to replace bdrv_delete() later. Signed-off-by: Fam Zheng f...@redhat.com --- block.c | 21 + include/block/block.h | 2 ++ include/block/block_int.h | 1 + 3 files changed, 24 insertions(+) -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v3 4/7] block: make bdrv_delete() static
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v3 5/7] migration: omit drive ref as we have bdrv_ref now
There should be a section of code in device hot unplug, checking
DriverInfo's ref, fail or do nothing when ref != 1. But I haven't found
that code, so not sure whether this patch will change the behavior in
device hot unplug.
block-migration.c does not actually use DriveInfo anywhere. Hence it's
safe to drive ref code, we really only care about referencing BDS.
Signed-off-by: Fam Zheng f...@redhat.com
---
block-migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index f803f20..daf9ec1 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -336,8 +336,8 @@ static void init_blk_migration_it(void *opaque,
BlockDriverState *bs)
bmds-completed_sectors = 0;
bmds-shared_base = block_mig_state.shared_base;
alloc_aio_bitmap(bmds);
-drive_get_ref(drive_get_by_blockdev(bs));
bdrv_set_in_use(bs, 1);
+bdrv_ref(bs);
block_mig_state.total_sector_sum += sectors;
@@ -575,7 +575,7 @@ static void blk_mig_cleanup(void)
while ((bmds = QSIMPLEQ_FIRST(block_mig_state.bmds_list)) != NULL) {
QSIMPLEQ_REMOVE_HEAD(block_mig_state.bmds_list, entry);
bdrv_set_in_use(bmds-bs, 0);
-drive_put_ref(drive_get_by_blockdev(bmds-bs));
+bdrv_unref(bmds-bs);
g_free(bmds-aio_bitmap);
g_free(bmds);
}
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v3 6/7] xen_disk: simplify blk_disconnect with refcnt
Better to split it into two patches:
1 bugfix: always call bdrv_detach_dev().
2 use refcnt to manage lifecycle.
We call bdrv_attach_dev when initializing whether or not bs is created
locally, so call bdrv_detach_dev and let the refcnt handle the
lifecycle.
Signed-off-by: Fam Zheng f...@redhat.com
---
hw/block/xen_disk.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 99537e8..39757d9 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -812,6 +812,9 @@ static int blk_connect(struct XenDevice *xendev)
/* setup via qemu cmdline - already setup for us */
xen_be_printf(blkdev-xendev, 2, get configured bdrv (cmdline
setup)\n);
blkdev-bs = blkdev-dinfo-bdrv;
+/* blkdev-bs is not create by us, we get a reference
+ * so we can bdrv_unref() unconditionally */
+bdrv_ref(blkdev-bs);
}
bdrv_attach_dev_nofail(blkdev-bs, blkdev);
blkdev-file_size = bdrv_getlength(blkdev-bs);
@@ -910,12 +913,8 @@ static void blk_disconnect(struct XenDevice *xendev)
struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev,
xendev);
if (blkdev-bs) {
-if (!blkdev-dinfo) {
-/* close/delete only if we created it ourself */
-bdrv_close(blkdev-bs);
-bdrv_detach_dev(blkdev-bs, blkdev);
-bdrv_unref(blkdev-bs);
-}
+bdrv_detach_dev(blkdev-bs, blkdev);
+bdrv_unref(blkdev-bs);
blkdev-bs = NULL;
}
xen_be_unbind_evtchn(blkdev-xendev);
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v3 7/7] nbd: use BlockDriverState refcnt
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com Previously, nbd calls drive_get_ref() on the drive of bs. A BDS doesn't always have associated dinfo, which nbd doesn't care either. We already have BDS ref count, so use it to make it safe for a BDS w/o blockdev. Signed-off-by: Fam Zheng f...@redhat.com -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V8 00/13] monitor: support sub command group in auto completion and help
于 2013-7-31 0:03, Luiz Capitulino 写道: On Fri, 26 Jul 2013 11:20:29 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: This series make auto completion and help functions works normal for sub command, by using reentrant functions. In order to do that, global variables are not directly used in those functions any more. With this series, cmd_table is a member of structure Monitor so it is possible to create a monitor with different command table now, auto completion will work in that monitor. In short, info is not treated as a special case now, this series ensure help and auto complete function works normal for any sub command added in the future. Patch 5 replaced cur_mon with rs-mon, it is safe because: monitor_init() calls readline_init() which initialize mon-rs, result is mon-rs-mon == mon. Then qemu_chr_add_handlers() is called, which make monitor_read() function take *mon as its opaque. Later, when user input, monitor_read() is called, where cur_mon is set to *mon by cur_mon = opaque. If qemu's monitors run in one thread, then later in readline_handle_byte() and readline_comletion(), cur_mon is actually equal to rs-mon, in another word, it points to the monitor instance, so it is safe to replace *cur_mon in those functions. I've applied this to qmp-next with the change I suggested for patch 09/13. Thanks a lot! Thanks for Luiz and Eric for reviewing. V2: General: To discard *info_comds more graceful, help related function is modified to support sub command too. Patch 6/7 are added to improve help related functions. Patch 5: not directly return to make sure args are freed. Address Luiz's comments: Split patch into small series. struct mon_cmd_t was not moved into header file, instead mon_cmnd_t *cmd_table is added as a member in struct Monitor. 5/7: drop original code comments for info in monitor_find_completion(). v3: 5/7: add parameter **args_cmdline in parse_cmdline() to tell next valid parameter's position. This fix the issue in case command length in input is not equal to its name's length such as help|?, and the case input start with space such as s. 7/7: better commit message. v4: Address Eric's comments: 1/7, 2/7, 4/7: better commit title and message. 1/7 remove useless (char *) in old code, add space around for () in old code. 3/7: separate code moving patch before usage. 4/7: add space around for () in old code, add min(nb_args, MAX_ARGS) in free to make code stronger. v5: 4/7: use a b ? a : b instead of macro min. v6: Address Luiz's comments: 1/13 ~ 5/13: splitted small patches. 5/13: added commit message about the correctness of replacing of cur_mon and test result. 6/13: better comments in code. 7/13: added commit message about the reason of code moving. 8/13: new patch to improve parse_cmdline(), since it is a more generic function now. 9/13: reworked the commit message, better commentes in code, use free_cmdline_args() in clean. It is a bit hard to split this patch into smaller meaning ful ones, so kepted this patch as a relative larger one, with better commit message. 12/13: put case 'S' with case 's' in monitor_find_completion_by_table(). moved this patch ahead of patch 13/13. 13/13: this patch is moved behind patch 12/13. Generic change: 10/13: splitted patch which moved out the reentrant part into a separate function, make review easier. This also avoided re-parsing the command line which does in previous version. 11/13: splitted patch, which simply remove usage of info_cmds and support sub command by re-enter the function. v7: Address Luiz's comments: 5/13: moved the comments why the change is safe, to cover-letter. 8/13: use assert in free_cmdline_args(), fail when args in input exceed the limit in parse_cmdline(). v8: Address Eric's comments: Fix typo in commit messages. Wenchao Xia (13): 1 monitor: avoid use of global *cur_mon in cmd_completion() 2 monitor: avoid use of global *cur_mon in file_completion() 3 monitor: avoid use of global *cur_mon in block_completion_it() 4 monitor: avoid use of global *cur_mon in monitor_find_completion() 5 monitor: avoid use of global *cur_mon in readline_completion() 6 monitor: avoid direct use of global variable *mon_cmds 7 monitor: code move for parse_cmdline() 8 monitor: refine parse_cmdline() 9 monitor: support sub command in help 10 monitor: refine monitor_find_completion() 11 monitor: support sub command in auto completion 12 monitor: allow help show message for single command in sub group 13 monitor: improve auto complete of help for single command in sub group hmp-commands.hx|2 +- include/monitor/readline.h |3 +- monitor.c | 438 readline.c |5 +- 4 files changed, 289 insertions(+), 159 deletions(-) -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V8 09/13] monitor: support sub command in help
于 2013-7-30 22:51, Luiz Capitulino 写道:
On Fri, 26 Jul 2013 11:20:38 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
The old code in help_cmd() uses global 'info_cmds' and treats it as a
special case. Actually 'info_cmds' is a sub command group of 'mon_cmds',
in order to avoid direct use of it, help_cmd() needs to change its work
mechanism to support sub command and not treat it as a special case
any more.
To support sub command, help_cmd() will first parse the input and then call
help_cmd_dump(), which works as a reentrant function. When it meets a sub
command, it simply enters the function again. Since help dumping needs to
know whole input to printf full help message include prefix, for example,
help info block need to printf prefix info, so help_cmd_dump() takes all
args from input and extra parameter arg_index to identify the progress.
Another function help_cmd_dump_one() is introduced to printf the prefix
and command's help message.
Now help supports sub command, so later if another sub command group is
added in any depth, help will automatically work for it. Still help info
block will show error since command parser reject additional parameter,
which can be improved later. log is still treated as a special case.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
monitor.c | 63 +++-
1 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/monitor.c b/monitor.c
index c942b77..77df88d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -868,33 +868,76 @@ static int parse_cmdline(const char *cmdline,
return -1;
}
+static void help_cmd_dump_one(Monitor *mon,
+ const mon_cmd_t *cmd,
+ char **prefix_args,
+ int prefix_args_nb)
+{
+int i;
+
+for (i = 0; i prefix_args_nb; i++) {
+monitor_printf(mon, %s , prefix_args[i]);
+}
+monitor_printf(mon, %s %s -- %s\n, cmd-name, cmd-params, cmd-help);
+}
+
+/* @args[@arg_index] is the valid command need to find in @cmds */
static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
- const char *prefix, const char *name)
+ char **args, int nb_args, int arg_index)
{
const mon_cmd_t *cmd;
-for(cmd = cmds; cmd-name != NULL; cmd++) {
-if (!name || !strcmp(name, cmd-name))
-monitor_printf(mon, %s%s %s -- %s\n, prefix, cmd-name,
- cmd-params, cmd-help);
+/* No valid arg need to compare with, dump all in *cmds */
+if (arg_index = nb_args) {
+for (cmd = cmds; cmd-name != NULL; cmd++) {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+return;
+}
+
+/* Find one entry to dump */
+for (cmd = cmds; cmd-name != NULL; cmd++) {
+if (compare_cmd(args[arg_index], cmd-name)) {
+if (cmd-sub_table) {
+/* continue with next arg */
+help_cmd_dump(mon, cmd-sub_table,
+ args, nb_args, arg_index + 1);
+} else {
+help_cmd_dump_one(mon, cmd, args, arg_index);
+}
+break;
+}
}
}
static void help_cmd(Monitor *mon, const char *name)
{
-if (name !strcmp(name, info)) {
-help_cmd_dump(mon, info_cmds, info , NULL);
-} else {
-help_cmd_dump(mon, mon-cmd_table, , name);
-if (name !strcmp(name, log)) {
+char *args[MAX_ARGS];
+int nb_args = 0;
+
+/* 1. parse user input */
+if (name) {
+/* special case for log, directly dump and return */
+if (!strcmp(name, log)) {
const QEMULogItem *item;
monitor_printf(mon, Log items (comma separated):\n);
monitor_printf(mon, %-10s %s\n, none, remove all logs);
for (item = qemu_log_items; item-mask != 0; item++) {
monitor_printf(mon, %-10s %s\n, item-name, item-help);
}
+return;
+}
+
+if (parse_cmdline(name, nb_args, args) 0) {
+goto cleanup;
Won't this result in a double-free if parse_cmdline() fails?
The fix is just to return instead of going to cleanup. I can do this
change if you agree and if I don't spot another bug.
nb_args = 0 in that case, I think it would not free again. Still, I
am OK to do this change, since the parse_cmdline() already does the
clean up, not need to ask caller to do it again.
}
}
+
+/* 2. dump the contents according to parsed args */
+help_cmd_dump(mon, mon-cmd_table, args, nb_args, 0);
+
+cleanup:
+free_cmdline_args(args, nb_args);
}
static void do_help_cmd(Monitor *mon, const QDict *qdict)
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 02/18] block: stop relying on io_flush() in bdrv_drain_all()
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v6 03/18] dataplane/virtio-blk: check exit conditions before aio_poll()
Check exit conditions before entering blocking aio_poll(). This is
mainly for consistency since it's unlikely that we are stopping in the
first event loop iteration.
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
hw/block/dataplane/virtio-blk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 2faed43..8d3e145 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -379,9 +379,9 @@ static void *data_plane_thread(void *opaque)
{
VirtIOBlockDataPlane *s = opaque;
-do {
+while (!s-stopping || s-num_reqs 0) {
aio_poll(s-ctx, true);
-} while (!s-stopping || s-num_reqs 0);
+}
return NULL;
}
It seems more likely a bug fix.
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 04/18] tests: adjust test-aio to new aio_poll() semantics
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
aio_poll(ctx, true) will soon block if any fd handlers have been set.
Previously it would only block when .io_flush() returned true.
This means that callers must check their wait condition *before*
aio_poll() to avoid deadlock.
Reviewed-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
tests/test-aio.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/tests/test-aio.c b/tests/test-aio.c
index c173870..20bf5e6 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -15,6 +15,13 @@
AioContext *ctx;
+typedef struct {
+EventNotifier e;
+int n;
+int active;
+bool auto_set;
+} EventNotifierTestData;
+
/* Wait until there are no more BHs or AIO requests */
static void wait_for_aio(void)
{
@@ -23,6 +30,14 @@ static void wait_for_aio(void)
}
}
+/* Wait until event notifier becomes inactive */
+static void wait_until_inactive(EventNotifierTestData *data)
+{
+while (data-active 0) {
+aio_poll(ctx, true);
+}
+}
+
/* Simple callbacks for testing. */
typedef struct {
@@ -50,13 +65,6 @@ static void bh_delete_cb(void *opaque)
}
}
-typedef struct {
-EventNotifier e;
-int n;
-int active;
-bool auto_set;
-} EventNotifierTestData;
-
static int event_active_cb(EventNotifier *e)
{
EventNotifierTestData *data = container_of(e, EventNotifierTestData, e);
@@ -281,7 +289,7 @@ static void test_flush_event_notifier(void)
g_assert_cmpint(data.active, ==, 9);
g_assert(aio_poll(ctx, false));
-wait_for_aio();
+wait_until_inactive(data);
g_assert_cmpint(data.n, ==, 10);
g_assert_cmpint(data.active, ==, 0);
g_assert(!aio_poll(ctx, false));
@@ -325,7 +333,7 @@ static void test_wait_event_notifier_noflush(void)
g_assert_cmpint(data.n, ==, 2);
event_notifier_set(dummy.e);
-wait_for_aio();
+wait_until_inactive(dummy);
g_assert_cmpint(data.n, ==, 2);
g_assert_cmpint(dummy.n, ==, 1);
g_assert_cmpint(dummy.active, ==, 0);
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 05/18] tests: adjust test-thread-pool to new aio_poll() semantics
I am confused for sometime about the reason of the change, I think
it can be concluded as: The meaning of aio_poll() will be changed, so
change the caller in this test case.
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
aio_poll(ctx, true) will soon block when fd handlers have been set.
Previously aio_poll() would return early if all .io_flush() returned
false. This means we need to check the equivalent of the .io_flush()
condition *before* calling aio_poll(ctx, true) to avoid deadlock.
Reviewed-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
tests/test-thread-pool.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
index b62338f..8188d1a 100644
--- a/tests/test-thread-pool.c
+++ b/tests/test-thread-pool.c
@@ -40,19 +40,13 @@ static void done_cb(void *opaque, int ret)
active--;
}
-/* Wait until all aio and bh activity has finished */
-static void qemu_aio_wait_all(void)
-{
-while (aio_poll(ctx, true)) {
-/* Do nothing */
-}
-}
-
static void test_submit(void)
{
WorkerTestData data = { .n = 0 };
thread_pool_submit(pool, worker_cb, data);
-qemu_aio_wait_all();
+while (data.n == 0) {
+aio_poll(ctx, true);
+}
g_assert_cmpint(data.n, ==, 1);
}
@@ -65,7 +59,9 @@ static void test_submit_aio(void)
/* The callbacks are not called until after the first wait. */
active = 1;
g_assert_cmpint(data.ret, ==, -EINPROGRESS);
-qemu_aio_wait_all();
+while (data.ret == -EINPROGRESS) {
+aio_poll(ctx, true);
+}
g_assert_cmpint(active, ==, 0);
g_assert_cmpint(data.n, ==, 1);
g_assert_cmpint(data.ret, ==, 0);
@@ -103,7 +99,9 @@ static void test_submit_co(void)
/* qemu_aio_wait_all will execute the rest of the coroutine. */
-qemu_aio_wait_all();
+while (data.ret == -EINPROGRESS) {
+aio_poll(ctx, true);
+}
/* Back here after the coroutine has finished. */
@@ -187,7 +185,9 @@ static void test_cancel(void)
}
/* Finish execution and execute any remaining callbacks. */
-qemu_aio_wait_all();
+while (active 0) {
+aio_poll(ctx, true);
+}
g_assert_cmpint(active, ==, 0);
for (i = 0; i 100; i++) {
if (data[i].n == 3) {
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 06/18] aio: stop using .io_flush()
Now that aio_poll() users check their termination condition themselves, it is no longer necessary to call .io_flush() handlers. The behavior of aio_poll() changes as follows: 1. .io_flush() is no longer invoked and file descriptors are *always* monitored. Previously returning 0 from .io_flush() would skip this file descriptor. Due to this change it is essential to check that requests are pending before calling qemu_aio_wait(). Failure to do so means we block, for example, waiting for an idle iSCSI socket to become readable when there are no requests. Currently all qemu_aio_wait()/aio_poll() callers check before calling. 2. aio_poll() now returns true if progress was made (BH or fd handlers executed) and false otherwise. Previously it would return true whenever 'busy', which means that .io_flush() returned true. The 'busy' concept no longer exists so just progress is returned. Due to this change we need to update tests/test-aio.c which asserts aio_poll() return values. Note that QEMU doesn't actually rely on these return values so only tests/test-aio.c cares. Note that ctx-notifier, the EventNotifier fd used for aio_notify(), is now handled as a special case. This is a little ugly but maintains aio_poll() semantics, i.e. aio_notify() does not count as 'progress' and aio_poll() avoids blocking when the user has not set any fd handlers yet. I guess the goal is, distinguish qemu's internal used fd, with the real meaningful external fd such as socket? How about distinguish them with different GSource? Patches after this remove .io_flush() handler code until we can finally drop the io_flush arguments to aio_set_fd_handler() and friends. Reviewed-by: Paolo Bonzini pbonz...@redhat.com Signed-off-by: Stefan Hajnoczi stefa...@redhat.com --- aio-posix.c | 29 + aio-win32.c | 34 ++ tests/test-aio.c | 10 +- 3 files changed, 28 insertions(+), 45 deletions(-) -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v6 07/18] block/curl: drop curl_aio_flush()
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com .io_flush() is no longer called so drop curl_aio_flush(). The acb[] array that the function checks is still used in other parts of block/curl.c. Therefore we cannot remove acb[], it is needed. Signed-off-by: Stefan Hajnoczi stefa...@redhat.com --- block/curl.c | 22 +++--- 1 file changed, 3 insertions(+), 19 deletions(-) -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v6 08/18] block/gluster: drop qemu_gluster_aio_flush_cb()
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com Since .io_flush() is no longer called we do not need qemu_gluster_aio_flush_cb() anymore. It turns out that qemu_aio_count is unused now and can be dropped. Signed-off-by: Stefan Hajnoczi stefa...@redhat.com --- block/gluster.c | 16 +--- 1 file changed, 1 insertion(+), 15 deletions(-) -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v6 09/18] block/iscsi: drop iscsi_process_flush()
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
.io_flush() is no longer called so drop iscsi_process_flush().
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
block/iscsi.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 5f28c6a..e2692d6 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -146,13 +146,6 @@ static const AIOCBInfo iscsi_aiocb_info = {
static void iscsi_process_read(void *arg);
static void iscsi_process_write(void *arg);
-static int iscsi_process_flush(void *arg)
-{
-IscsiLun *iscsilun = arg;
-
-return iscsi_queue_length(iscsilun-iscsi) 0;
-}
-
static void
iscsi_set_events(IscsiLun *iscsilun)
{
@@ -166,7 +159,7 @@ iscsi_set_events(IscsiLun *iscsilun)
qemu_aio_set_fd_handler(iscsi_get_fd(iscsi),
iscsi_process_read,
(ev POLLOUT) ? iscsi_process_write : NULL,
- iscsi_process_flush,
+ NULL,
iscsilun);
}
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 11/18] block/nbd: drop nbd_have_request()
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
.io_flush() is no longer called so drop nbd_have_request(). We cannot
drop in_flight since it is still used by other block/nbd.c code.
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
block/nbd.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 9c480b8..f1619f9 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -279,13 +279,6 @@ static void nbd_coroutine_start(BDRVNBDState *s, struct
nbd_request *request)
request-handle = INDEX_TO_HANDLE(s, i);
}
-static int nbd_have_request(void *opaque)
-{
-BDRVNBDState *s = opaque;
-
-return s-in_flight 0;
-}
-
static void nbd_reply_ready(void *opaque)
{
BDRVNBDState *s = opaque;
@@ -342,7 +335,7 @@ static int nbd_co_send_request(BDRVNBDState *s, struct
nbd_request *request,
qemu_co_mutex_lock(s-send_mutex);
s-send_coroutine = qemu_coroutine_self();
qemu_aio_set_fd_handler(s-sock, nbd_reply_ready, nbd_restart_write,
-nbd_have_request, s);
+NULL, s);
if (qiov) {
if (!s-is_unix) {
socket_set_cork(s-sock, 1);
@@ -362,7 +355,7 @@ static int nbd_co_send_request(BDRVNBDState *s, struct
nbd_request *request,
rc = nbd_send_request(s-sock, request);
}
qemu_aio_set_fd_handler(s-sock, nbd_reply_ready, NULL,
-nbd_have_request, s);
+NULL, s);
s-send_coroutine = NULL;
qemu_co_mutex_unlock(s-send_mutex);
return rc;
@@ -439,7 +432,7 @@ static int nbd_establish_connection(BlockDriverState *bs)
* kick the reply mechanism. */
qemu_set_nonblock(sock);
qemu_aio_set_fd_handler(sock, nbd_reply_ready, NULL,
-nbd_have_request, s);
+NULL, s);
s-sock = sock;
s-size = size;
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 10/18] block/linux-aio: drop qemu_laio_completion_cb()
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
.io_flush() is no longer called so drop qemu_laio_completion_cb(). It
turns out that count is now unused so drop that too.
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
block/linux-aio.c | 17 ++---
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/block/linux-aio.c b/block/linux-aio.c
index ee0f8d1..d9128f3 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -39,7 +39,6 @@ struct qemu_laiocb {
struct qemu_laio_state {
io_context_t ctx;
EventNotifier e;
-int count;
};
static inline ssize_t io_event_ret(struct io_event *ev)
@@ -55,8 +54,6 @@ static void qemu_laio_process_completion(struct
qemu_laio_state *s,
{
int ret;
-s-count--;
-
ret = laiocb-ret;
if (ret != -ECANCELED) {
if (ret == laiocb-nbytes) {
@@ -101,13 +98,6 @@ static void qemu_laio_completion_cb(EventNotifier *e)
}
}
-static int qemu_laio_flush_cb(EventNotifier *e)
-{
-struct qemu_laio_state *s = container_of(e, struct qemu_laio_state, e);
-
-return (s-count 0) ? 1 : 0;
-}
-
static void laio_cancel(BlockDriverAIOCB *blockacb)
{
struct qemu_laiocb *laiocb = (struct qemu_laiocb *)blockacb;
@@ -177,14 +167,11 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs,
void *aio_ctx, int fd,
goto out_free_aiocb;
}
io_set_eventfd(laiocb-iocb, event_notifier_get_fd(s-e));
-s-count++;
if (io_submit(s-ctx, 1, iocbs) 0)
-goto out_dec_count;
+goto out_free_aiocb;
return laiocb-common;
-out_dec_count:
-s-count--;
out_free_aiocb:
qemu_aio_release(laiocb);
return NULL;
@@ -204,7 +191,7 @@ void *laio_init(void)
}
qemu_aio_set_event_notifier(s-e, qemu_laio_completion_cb,
-qemu_laio_flush_cb);
+NULL);
return s;
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 12/18] block/rbd: drop qemu_rbd_aio_flush_cb()
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
.io_flush() is no longer called so drop qemu_rbd_aio_flush_cb().
qemu_aio_count is unused now so drop it too.
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
block/rbd.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index cb71751..71b4a0c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -100,7 +100,6 @@ typedef struct BDRVRBDState {
rados_ioctx_t io_ctx;
rbd_image_t image;
char name[RBD_MAX_IMAGE_NAME_SIZE];
-int qemu_aio_count;
char *snap;
int event_reader_pos;
RADOSCB *event_rcb;
@@ -428,19 +427,11 @@ static void qemu_rbd_aio_event_reader(void *opaque)
if (s-event_reader_pos == sizeof(s-event_rcb)) {
s-event_reader_pos = 0;
qemu_rbd_complete_aio(s-event_rcb);
-s-qemu_aio_count--;
}
}
} while (ret 0 errno == EINTR);
}
-static int qemu_rbd_aio_flush_cb(void *opaque)
-{
-BDRVRBDState *s = opaque;
-
-return (s-qemu_aio_count 0);
-}
-
/* TODO Convert to fine grained options */
static QemuOptsList runtime_opts = {
.name = rbd,
@@ -554,7 +545,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags)
fcntl(s-fds[0], F_SETFL, O_NONBLOCK);
fcntl(s-fds[1], F_SETFL, O_NONBLOCK);
qemu_aio_set_fd_handler(s-fds[RBD_FD_READ], qemu_rbd_aio_event_reader,
-NULL, qemu_rbd_aio_flush_cb, s);
+NULL, NULL, s);
qemu_opts_del(opts);
@@ -741,8 +732,6 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState
*bs,
off = sector_num * BDRV_SECTOR_SIZE;
size = nb_sectors * BDRV_SECTOR_SIZE;
-s-qemu_aio_count++; /* All the RADOSCB */
-
rcb = g_malloc(sizeof(RADOSCB));
rcb-done = 0;
rcb-acb = acb;
@@ -779,7 +768,6 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState
*bs,
failed:
g_free(rcb);
-s-qemu_aio_count--;
qemu_aio_release(acb);
return NULL;
}
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH v6 04/18] tests: adjust test-aio to new aio_poll() semantics
Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
aio_poll(ctx, true) will soon block if any fd handlers have been set.
Previously it would only block when .io_flush() returned true.
This means that callers must check their wait condition *before*
aio_poll() to avoid deadlock.
Reviewed-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
tests/test-aio.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/tests/test-aio.c b/tests/test-aio.c
index c173870..20bf5e6 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -15,6 +15,13 @@
AioContext *ctx;
+typedef struct {
+EventNotifier e;
+int n;
+int active;
+bool auto_set;
+} EventNotifierTestData;
+
/* Wait until there are no more BHs or AIO requests */
static void wait_for_aio(void)
{
@@ -23,6 +30,14 @@ static void wait_for_aio(void)
}
}
+/* Wait until event notifier becomes inactive */
+static void wait_until_inactive(EventNotifierTestData *data)
+{
+while (data-active 0) {
+aio_poll(ctx, true);
+}
+}
+
/* Simple callbacks for testing. */
typedef struct {
@@ -50,13 +65,6 @@ static void bh_delete_cb(void *opaque)
}
}
-typedef struct {
-EventNotifier e;
-int n;
-int active;
-bool auto_set;
-} EventNotifierTestData;
-
static int event_active_cb(EventNotifier *e)
{
EventNotifierTestData *data = container_of(e, EventNotifierTestData, e);
@@ -281,7 +289,7 @@ static void test_flush_event_notifier(void)
g_assert_cmpint(data.active, ==, 9);
g_assert(aio_poll(ctx, false));
-wait_for_aio();
+wait_until_inactive(data);
g_assert_cmpint(data.n, ==, 10);
g_assert_cmpint(data.active, ==, 0);
g_assert(!aio_poll(ctx, false));
@@ -325,7 +333,7 @@ static void test_wait_event_notifier_noflush(void)
g_assert_cmpint(data.n, ==, 2);
event_notifier_set(dummy.e);
-wait_for_aio();
+wait_until_inactive(dummy);
g_assert_cmpint(data.n, ==, 2);
g_assert_cmpint(dummy.n, ==, 1);
g_assert_cmpint(dummy.active, ==, 0);
--
Best Regards
Wenchao Xia
