Re: [libvirt] [PATCH] Return a suitable error message if we can't find a matching emulator

[Guido Günther]

On Mon, Oct 11, 2010 at 03:39:39PM -0600, Eric Blake wrote:
 On 10/08/2010 07:55 AM, Guido Günther wrote:
 Hi,
 attached patch improves the error message when we can't find a suitable
 emulator. Otherwise it's simply Unknown failure. O.k. to apply?
 Cheers,
   -- Guido
 
 ACK.
Pushed, thanks.
 -- Guido

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/8] virsh: better support double quote

[Lai Jiangshan]

In origin code, double quote is only allowed at the begin or end
complicated argument
--some_opt=complicated string  (we split this argument into 2 parts,
option and data, the data is complicated string).

This patch makes it allow double quote at any position of
an argument:
complicated argument
complicated argument
--some opt=complicated string

This patch is also needed for the following patches,
the following patches will not split option argument into 2 parts,
so we have to allow double quote at any position of an argument.

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index 57ea618..7b6f2b6 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10172,9 +10172,9 @@ static int
 vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
 {
 int tk = VSH_TK_NONE;
-int quote = FALSE;
+bool double_quote = false;
 int sz = 0;
-char *p = str;
+char *p = str, *q;
 char *tkstr = NULL;
 
 *end = NULL;
@@ -10188,9 +10188,13 @@ vshCommandGetToken(vshControl *ctl, char *str, char 
**end, char **res)
 *end = ++p; /* = \0 or begin of next command */
 return VSH_TK_END;
 }
+
+q = p;
+*res = NULL;
+copy:
 while (*p) {
 /* end of token is blank space or ';' */
-if ((quote == FALSE  (*p == ' ' || *p == '\t')) || *p == ';')
+if (!double_quote  (*p == ' ' || *p == '\t' || *p == ';'))
 break;
 
 /* end of option name could be '=' */
@@ -10206,23 +10210,22 @@ vshCommandGetToken(vshControl *ctl, char *str, char 
**end, char **res)
 p += 2;
 } else {
 tk = VSH_TK_DATA;
-if (*p == '') {
-quote = TRUE;
-p++;
-} else {
-quote = FALSE;
-}
 }
 tkstr = p;  /* begin of token */
-} else if (quote  *p == '') {
-quote = FALSE;
+}
+
+   if (*p == '') {
+double_quote = !double_quote;
 p++;
-break;  /* end of ... token */
+continue;
 }
+
+if (*res)
+(*res)[sz] = *p;
 p++;
 sz++;
 }
-if (quote) {
+if (double_quote) {
 vshError(ctl, %s, _(missing \));
 return VSH_TK_ERROR;
 }
@@ -10231,8 +10234,12 @@ vshCommandGetToken(vshControl *ctl, char *str, char 
**end, char **res)
 if (sz == 0)
 return VSH_TK_END;
 
-*res = vshMalloc(ctl, sz + 1);
-memcpy(*res, tkstr, sz);
+if (!*res) {
+*res = vshMalloc(ctl, sz + 1);
+sz = 0;
+p = q;
+goto copy;
+}
 *(*res + sz) = '\0';
 
 *end = p;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH V3 0/8] virsh: rework command parsing

[Lai Jiangshan]

Old virsh command parsing mashes all the args back into a string and
miss the quotes, this patches fix it. It is also needed for introducing
qemu-monitor-command which is very useful.

This patches add vrshCommandParser abstraction to parser the args.
For command string, we use vshCommandStringParse()
For command argument vector, we use vshCommandArgvParse()

And the usage was changed:
old:
virsh [options] [commands]

new:
virsh [options]... [command_string]
virsh [options]... command [args...]

So we still support commands like:
# virsh define D.xml; dumpxml D
define D.xml; dumpxml D was parsed as a commands-string.

and support commands like:
# virsh qemu-monitor-command f13guest info cpus
we will not mash them into a string, we use new argv parser for it.

But we don't support the command like:
# virsh define D.xml; dumpxml D
define D.xml; dumpxml was parsed as a command-name, but we have no such 
command-name.

Misc changed behavior:
1) support single quote
2) support escape '\'
3) a better double quoting support, the following commands are now supported:
 virsh # dumpxml --update-cpu vm1
 virsh # dumpxml --update-cpu vm1
4) better handling the boolean options, in old code the following commands are 
equivalent: 
 virsh # dumpxml --update-cpu=vm1
 virsh # dumpxml --update-cpu vm1
   after this patch applied, the first one will become illegal.
5) support --

The idea of this patch is from Daniel P. Berrange.

changed from V1:
changed the usage as Eric Blake suggested.
changed from V2
new vrshCommandParser abstraction
apply Eric Blake's comments.

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
 virsh.c |  259 +---
 1 file changed, 152 insertions(+), 107 deletions(-)

---
I was preparing for linuxcon japan and attended it and took a long vacation
after it, very late for V3.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/8] virsh: allow zero length arguments

[Lai Jiangshan]

the following command is allowed at shell, we also make it allowed at virsh 
shel.
# somecmd 

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index 7b6f2b6..1e95023 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10175,7 +10175,6 @@ vshCommandGetToken(vshControl *ctl, char *str, char 
**end, char **res)
 bool double_quote = false;
 int sz = 0;
 char *p = str, *q;
-char *tkstr = NULL;
 
 *end = NULL;
 
@@ -10211,7 +10210,6 @@ copy:
 } else {
 tk = VSH_TK_DATA;
 }
-tkstr = p;  /* begin of token */
 }
 
if (*p == '') {
@@ -10229,10 +10227,6 @@ copy:
 vshError(ctl, %s, _(missing \));
 return VSH_TK_ERROR;
 }
-if (tkstr == NULL || *tkstr == '\0' || p == NULL)
-return VSH_TK_END;
-if (sz == 0)
-return VSH_TK_END;
 
 if (!*res) {
 *res = vshMalloc(ctl, sz + 1);

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 6/8] virsh: add escaper \ for command string parsing

[Lai Jiangshan]

add escaper \ for command string parsing, example:

virsh # cd /path/which/have/a/double\quote

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index 9fd0602..b96071d 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10235,7 +10235,11 @@ copy:
 if (!double_quote  (*p == ' ' || *p == '\t' || *p == ';'))
 break;
 
-   if (*p == '') {
+if (*p == '\\') { /* escape */
+p++;
+if (*p == '\0')
+break;
+} else if (*p == '') { /* double quote */
 double_quote = !double_quote;
 p++;
 continue;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/8] virsh: better handling the boolean option

[Lai Jiangshan]

in old code the following commands are equivalent:
 virsh # dumpxml --update-cpu=vm1
 virsh # dumpxml --update-cpu vm1
because the old code split the option argument into 2 parts:
--update-cpu=vm1 is split into update-cpu and vm1,
and update-cpu is a boolean option, so the parser takes vm1 as another
argument, very strange.

after this patch applied, the first one will become illegal.

To achieve this, we don't parse/check options when parsing command sting,
but check options when parsing a command argument. And the argument is
not split when parsing command sting.

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index 1e95023..f97ee42 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10164,14 +10164,12 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
  */
 #define VSH_TK_ERROR-1
 #define VSH_TK_NONE0
-#define VSH_TK_OPTION1
-#define VSH_TK_DATA2
-#define VSH_TK_END3
+#define VSH_TK_DATA1
+#define VSH_TK_END2
 
 static int
 vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
 {
-int tk = VSH_TK_NONE;
 bool double_quote = false;
 int sz = 0;
 char *p = str, *q;
@@ -10196,22 +10194,6 @@ copy:
 if (!double_quote  (*p == ' ' || *p == '\t' || *p == ';'))
 break;
 
-/* end of option name could be '=' */
-if (tk == VSH_TK_OPTION  *p == '=') {
-p++;/* skip '=' */
-break;
-}
-
-if (tk == VSH_TK_NONE) {
-if (*p == '-'  *(p + 1) == '-'  *(p + 2)
- c_isalnum(*(p + 2))) {
-tk = VSH_TK_OPTION;
-p += 2;
-} else {
-tk = VSH_TK_DATA;
-}
-}
-
if (*p == '') {
 double_quote = !double_quote;
 p++;
@@ -10237,7 +10219,7 @@ copy:
 *(*res + sz) = '\0';
 
 *end = p;
-return tk;
+return VSH_TK_DATA;
 }
 
 static int
@@ -10296,18 +10278,28 @@ vshCommandParse(vshControl *ctl, char *cmdstr)
 goto syntaxError;   /* ... or ignore this command only? */
 }
 VIR_FREE(tkdata);
-} else if (tk == VSH_TK_OPTION) {
-if (!(opt = vshCmddefGetOption(cmd, tkdata))) {
+} else if (*tkdata == '-'  *(tkdata + 1) == '-'  *(tkdata + 2)
+c_isalnum(*(tkdata + 2))) {
+char *optstr = strchr(tkdata + 2, '=');
+if (optstr) {
+*optstr = '\0'; /* conver the '=' to '\0' */
+optstr = vshStrdup(ctl, optstr + 1);
+}
+if (!(opt = vshCmddefGetOption(cmd, tkdata + 2))) {
 vshError(ctl,
  _(command '%s' doesn't support option --%s),
- cmd-name, tkdata);
+ cmd-name, tkdata + 2);
+VIR_FREE(optstr);
 goto syntaxError;
 }
-VIR_FREE(tkdata);   /* option name */
+VIR_FREE(tkdata);
 
 if (opt-type != VSH_OT_BOOL) {
 /* option data */
-tk = vshCommandGetToken(ctl, str, end, tkdata);
+if (optstr)
+tkdata = optstr;
+else
+tk = vshCommandGetToken(ctl, str, end, tkdata);
 str = end;
 if (tk == VSH_TK_ERROR)
 goto syntaxError;
@@ -10319,6 +10311,14 @@ vshCommandParse(vshControl *ctl, char *cmdstr)
  VSH_OT_INT ? _(number) : _(string));
 goto syntaxError;
 }
+} else {
+tkdata = NULL;
+if (optstr) {
+vshError(ctl, _(invalid '=' after option --%s),
+opt-name);
+VIR_FREE(optstr);
+goto syntaxError;
+}
 }
 } else if (tk == VSH_TK_DATA) {
 if (!(opt = vshCmddefGetData(cmd, data_ct++))) {
@@ -10344,8 +10344,8 @@ vshCommandParse(vshControl *ctl, char *cmdstr)
 vshDebug(ctl, 4, %s: %s(%s): %s\n,
  cmd-name,
  opt-name,
- tk == VSH_TK_OPTION ? _(OPTION) : _(DATA),
- arg-data);
+ opt-type != VSH_OT_BOOL ? _(optdata) : _(bool),
+ opt-type != VSH_OT_BOOL ? arg-data : _((none)));
 }
 if (!str)
 break;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/8] virsh: add vrshCommandParser abstraction

[Lai Jiangshan]

add vrshCommandParser and make vshCommandParse() accepts different
parsers.

the current code for parse command string is integrated as
vshCommandStringParse().

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
 virsh.c |   91 +++-
 1 file changed, 45 insertions(+), 46 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index f97ee42..27321a5 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10158,23 +10158,29 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
 return ret;
 }
 
+#define VSH_TK_ERROR   -1
+#define VSH_TK_ARG 0
+#define VSH_TK_SUBCMD_END  1
+#define VSH_TK_END 2
+
+typedef struct __vshCommandParser {
+int (*getNextArg)(vshControl *, struct __vshCommandParser *, char **);
+char *pos;
+} vshCommandParser;
+
+static int vshCommandParse(vshControl *ctl, vshCommandParser *parser);
+
 /* ---
  * Command string parsing
  * ---
  */
-#define VSH_TK_ERROR-1
-#define VSH_TK_NONE0
-#define VSH_TK_DATA1
-#define VSH_TK_END2
 
 static int
-vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
+vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res)
 {
 bool double_quote = false;
 int sz = 0;
-char *p = str, *q;
-
-*end = NULL;
+char *p = parser-pos, *q;
 
 while (p  *p  (*p == ' ' || *p == '\t'))
 p++;
@@ -10182,8 +10188,8 @@ vshCommandGetToken(vshControl *ctl, char *str, char 
**end, char **res)
 if (p == NULL || *p == '\0')
 return VSH_TK_END;
 if (*p == ';') {
-*end = ++p; /* = \0 or begin of next command */
-return VSH_TK_END;
+parser-pos = ++p; /* = \0 or begin of next command */
+return VSH_TK_SUBCMD_END;
 }
 
 q = p;
@@ -10218,14 +10224,25 @@ copy:
 }
 *(*res + sz) = '\0';
 
-*end = p;
-return VSH_TK_DATA;
+parser-pos = p;
+return VSH_TK_ARG;
+}
+
+static int vshCommandStringParse(vshControl *ctl, char *cmdstr)
+{
+vshCommandParser parser;
+
+if (cmdstr == NULL || *cmdstr == '\0')
+return FALSE;
+
+parser.pos = cmdstr;
+parser.getNextArg = vshCommandStringGetArg;
+return vshCommandParse(ctl, parser);
 }
 
 static int
-vshCommandParse(vshControl *ctl, char *cmdstr)
+vshCommandParse(vshControl *ctl, vshCommandParser *parser)
 {
-char *str;
 char *tkdata = NULL;
 vshCmd *clast = NULL;
 vshCmdOpt *first = NULL;
@@ -10235,44 +10252,27 @@ vshCommandParse(vshControl *ctl, char *cmdstr)
 ctl-cmd = NULL;
 }
 
-if (cmdstr == NULL || *cmdstr == '\0')
-return FALSE;
-
-str = cmdstr;
-while (str  *str) {
+for (;;) {
 vshCmdOpt *last = NULL;
 const vshCmdDef *cmd = NULL;
-int tk = VSH_TK_NONE;
+int tk;
 int data_ct = 0;
 
 first = NULL;
 
-while (tk != VSH_TK_END) {
-char *end = NULL;
+for (;;) {
 const vshCmdOptDef *opt = NULL;
 
 tkdata = NULL;
+tk = parser-getNextArg(ctl, parser, tkdata);
 
-/* get token */
-tk = vshCommandGetToken(ctl, str, end, tkdata);
-
-str = end;
-
-if (tk == VSH_TK_END) {
-VIR_FREE(tkdata);
-break;
-}
 if (tk == VSH_TK_ERROR)
 goto syntaxError;
+if (tk != VSH_TK_ARG)
+break;
 
 if (cmd == NULL) {
 /* first token must be command name */
-if (tk != VSH_TK_DATA) {
-vshError(ctl,
- _(unexpected token (command name): '%s'),
- tkdata);
-goto syntaxError;
-}
 if (!(cmd = vshCmddefSearch(tkdata))) {
 vshError(ctl, _(unknown command: '%s'), tkdata);
 goto syntaxError;   /* ... or ignore this command only? */
@@ -10299,11 +10299,10 @@ vshCommandParse(vshControl *ctl, char *cmdstr)
 if (optstr)
 tkdata = optstr;
 else
-tk = vshCommandGetToken(ctl, str, end, tkdata);
-str = end;
+tk = parser-getNextArg(ctl, parser, tkdata);
 if (tk == VSH_TK_ERROR)
 goto syntaxError;
-if (tk != VSH_TK_DATA) {
+if (tk != VSH_TK_ARG) {
 vshError(ctl,
  _(expected syntax: --%s %s),
  opt-name,
@@ -10320,7 +10319,7 @@ vshCommandParse(vshControl *ctl, char *cmdstr)
 goto syntaxError;
 }
 }
-} else if (tk == VSH_TK_DATA) {
+} else {
 if (!(opt = vshCmddefGetData(cmd, 

[libvirt] [PATCH 8/8] virsh: add -- support

[Lai Jiangshan]

-- means no option at the following arguments.

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index a5b438b..d01091f 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10305,6 +10305,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 vshCmdOpt *last = NULL;
 const vshCmdDef *cmd = NULL;
 int tk;
+bool data_only = false;
 int data_ct = 0;
 
 first = NULL;
@@ -10327,6 +10328,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 goto syntaxError;   /* ... or ignore this command only? */
 }
 VIR_FREE(tkdata);
+} else if (data_only) {
+goto get_data;
 } else if (*tkdata == '-'  *(tkdata + 1) == '-'  *(tkdata + 2)
 c_isalnum(*(tkdata + 2))) {
 char *optstr = strchr(tkdata + 2, '=');
@@ -10368,7 +10371,12 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 goto syntaxError;
 }
 }
+} else if (*tkdata == '-'  *(tkdata + 1) == '-'
+*(tkdata + 2) == '\0') {
+data_only = true;
+continue;
 } else {
+get_data:
 if (!(opt = vshCmddefGetData(cmd, data_ct++))) {
 vshError(ctl, _(unexpected data '%s'), tkdata);
 goto syntaxError;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/8] virsh: rework command parsing

[Lai Jiangshan]

Old virsh command parsing mashes all the args back into a string and
miss the quotes, this patches fix it. It is also needed for introducing
qemu-monitor-command which is very useful.

This patches uses the new vrshCommandParser abstraction and adds
vshCommandArgvParse() for arguments vector, so we don't need
to mash arguments vector into a command sting.

And the usage was changed:
old:
virsh [options] [commands]

new:
virsh [options]... [command_string]
virsh [options]... command [args...]

So we still support commands like:
# virsh define D.xml; dumpxml D
define D.xml; dumpxml D was parsed as a commands-string.

and support commands like:
# virsh qemu-monitor-command f13guest info cpus
we will not mash them into a string, we use new argv parser for it.

But we don't support the command like:
# virsh define D.xml; dumpxml D
define D.xml; dumpxml was parsed as a command-name, but we have no such 
command-name.

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
 virsh.c |   63 +++
 1 file changed, 43 insertions(+), 20 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 27321a5..9fd0602 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10165,12 +10165,47 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
 
 typedef struct __vshCommandParser {
 int (*getNextArg)(vshControl *, struct __vshCommandParser *, char **);
+/* vshCommandStringGetArg() */
 char *pos;
+/* vshCommandArgvGetArg() */
+char **arg_pos;
+char **arg_end;
 } vshCommandParser;
 
 static int vshCommandParse(vshControl *ctl, vshCommandParser *parser);
 
 /* ---
+ * Command argv parsing
+ * ---
+ */
+
+static int
+vshCommandArgvGetArg(vshControl *ctl, vshCommandParser *parser, char **res)
+{
+if (parser-arg_pos == parser-arg_end) {
+*res = NULL;
+return VSH_TK_END;
+}
+
+*res = vshStrdup(ctl, *parser-arg_pos);
+parser-arg_pos++;
+return VSH_TK_ARG;
+}
+
+static int vshCommandArgvParse(vshControl *ctl, int nargs, char **argv)
+{
+vshCommandParser parser;
+
+if (nargs = 0)
+return FALSE;
+
+parser.arg_pos = argv;
+parser.arg_end = argv + nargs;
+parser.getNextArg = vshCommandArgvGetArg;
+return vshCommandParse(ctl, parser);
+}
+
+/* ---
  * Command string parsing
  * ---
  */
@@ -10939,7 +10974,8 @@ static void
 vshUsage(void)
 {
 const vshCmdDef *cmd;
-fprintf(stdout, _(\n%s [options] [commands]\n\n
+fprintf(stdout, _(\n%s [options]... [command_string]
+  \n%s [options]... command [args...]\n\n
 options:\n
   -c | --connect urihypervisor connection URI\n
   -r | --readonly connect readonly\n
@@ -10949,7 +10985,7 @@ vshUsage(void)
   -t | --timing   print timing information\n
   -l | --log file   output logging to file\n
   -v | --version  program version\n\n
-commands (non interactive mode):\n), progname);
+commands (non interactive mode):\n), progname, 
progname);
 
 for (cmd = commands; cmd-name; cmd++)
 fprintf(stdout,
@@ -11069,26 +11105,13 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
 
 if (argc  end) {
 /* parse command */
-char *cmdstr;
-int sz = 0, ret;
-
 ctl-imode = FALSE;
-
-for (i = end; i  argc; i++)
-sz += strlen(argv[i]) + 1;  /* +1 is for blank space between items 
*/
-
-cmdstr = vshCalloc(ctl, sz + 1, 1);
-
-for (i = end; i  argc; i++) {
-strncat(cmdstr, argv[i], sz);
-sz -= strlen(argv[i]);
-strncat(cmdstr,  , sz--);
+if (argc - end == 1) {
+vshDebug(ctl, 2, commands: \%s\\n, argv[end]);
+return vshCommandStringParse(ctl, argv[end]);
+} else {
+return vshCommandArgvParse(ctl, argc - end, argv + end);
 }
-vshDebug(ctl, 2, command: \%s\\n, cmdstr);
-ret = vshCommandStringParse(ctl, cmdstr);
-
-VIR_FREE(cmdstr);
-return ret;
 }
 return TRUE;
 }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 7/8] virsh: support single quote

[Lai Jiangshan]

Some use may type command like this at the virsh shell:
virsh # somecmd 'some arg'

because some users often use single quote in linux shell.

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index b96071d..a5b438b 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10213,6 +10213,7 @@ static int vshCommandArgvParse(vshControl *ctl, int 
nargs, char **argv)
 static int
 vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res)
 {
+bool single_quote = false;
 bool double_quote = false;
 int sz = 0;
 char *p = parser-pos, *q;
@@ -10232,14 +10233,23 @@ vshCommandStringGetArg(vshControl *ctl, 
vshCommandParser *parser, char **res)
 copy:
 while (*p) {
 /* end of token is blank space or ';' */
-if (!double_quote  (*p == ' ' || *p == '\t' || *p == ';'))
+if (!double_quote  !single_quote
+ (*p == ' ' || *p == '\t' || *p == ';'))
 break;
 
-if (*p == '\\') { /* escape */
+if (!double_quote  *p == '\'') { /* single quote */
+single_quote = !single_quote;
+p++;
+continue;
+} else if (!single_quote  *p == '\\') { /* escape */
+/*
+ * The same as the bash, a \ in  is an escaper,
+ * but a \ in '' is not an escaper.
+ */
 p++;
 if (*p == '\0')
 break;
-} else if (*p == '') { /* double quote */
+} else if (!single_quote  *p == '') { /* double quote */
 double_quote = !double_quote;
 p++;
 continue;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] About the qemu networking

[杨树林]

Dear libvirt team,

 

I am testing the qemu with libvit networking, and I have a question.

 

First, I connected the switch and server via Ethernet, the link
encapsulation is 802.1q (eth1 is trunk link)

 

When I use sub-interface eth1.12 for vlan12,br12 bridged to eth1.12, and
then using tap0 ..tapN, the qemu guest networking works fine.

 

But when I use br1 bridged to eth1 without sub-interface, also use tap0…
tapn , it does not work. 

 

 

The topology as below:

 

  This works fine This doest not
work

 

Eth1 (trunk)   eth1(trunk)

|   | 

|--

-   |  

   |   |br1

   Eth1.12  eth1.13  … |

  |   |  ___

 br12 br13   |   |

  |   |  |   …   |

   |   | tap0   tapn

   -  --

   |  |   |   ... |

   |  |   ||

Tap0 …   tapn  tapn+1tapm

 

 

 

Is the second solution works ? if works? What should I do to ?

 

Thanks very much, I am look forward to hear from you. 

 

Best Regards

 

 

杨树林

技术保障中心 网络工程师

snda

上海盛大网络发展有限公司

上海浦东新区碧波路690号1号楼

邮编：201203

电话：021-50504740-896570

传真：021-50504740-895746

email：yangshu...@shandagames.com

网址：http://www.snda.com

 

image001.gif--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/4] xen: Fix device hot(un)plug

[Jiri Denemark]

   xen: Make xenDaemon*DeviceFlags errors less confusing
   xen: Fix logic bug in xenDaemon*DeviceFlags
   xen: xenXMDomain*DeviceFlags should obey all flags
   xen: Fix virDomain{At,De}tachDevice
 
  src/xen/xen_driver.c|   24 -
  src/xen/xend_internal.c |   51 --
  src/xen/xm_internal.c   |   14 +++-
  3 files changed, 57 insertions(+), 32 deletions(-)

Eric and Jim, thanks for the review and additional testing.

I pushed this series.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Enable support for nested SVM

[Daniel P. Berrange]

This enables support for nested SVM using the regular CPU
model/features block. If the CPU model or features include
'svm', then the '-enable-nesting' flag will be added to the
QEMU command line. Latest out of tree patches for nested
'vmx', no longer require the '-enable-nesting' flag. They
instead just look at the cpu features. Several of the models
already include svm support, but QEMU was just masking out
the svm bit silently. So this will enable SVM on such
models

* src/qemu/qemu_conf.h: flag for -enable-nesting
* src/qemu/qemu_conf.c: Use -enable-nesting if VMX or SVM are in
  the CPUID
* src/cpu/cpu.h, src/cpu/cpu.c: API to check for a named feature
* src/cpu/cpu_x86.c: x86 impl of feature check
* src/libvirt_private.syms: Add cpuHasFeature
* src/qemuhelptest.c: Add nesting flag where required
---
 src/cpu/cpu.c|   24 
 src/cpu/cpu.h|   11 +++
 src/cpu/cpu_x86.c|   29 +
 src/libvirt_private.syms |1 +
 src/qemu/qemu_conf.c |   21 +++--
 src/qemu/qemu_conf.h |1 +
 tests/qemuhelptest.c |   12 
 7 files changed, 93 insertions(+), 6 deletions(-)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index def6974..f509e1c 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -424,3 +424,27 @@ cpuUpdate(virCPUDefPtr guest,
 
 return driver-update(guest, host);
 }
+
+bool
+cpuHasFeature(const char *arch,
+  const union cpuData *data,
+  const char *feature)
+{
+struct cpuArchDriver *driver;
+
+VIR_DEBUG(arch=%s, data=%p, feature=%s,
+  arch, data, feature);
+
+if ((driver = cpuGetSubDriver(arch)) == NULL)
+return -1;
+
+if (driver-hasFeature == NULL) {
+virCPUReportError(VIR_ERR_NO_SUPPORT,
+_(cannot check guest CPU data for %s architecture),
+  arch);
+return -1;
+}
+
+return driver-hasFeature(data, feature);
+}
+
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index a745917..3a7b996 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -82,6 +82,10 @@ typedef int
 (*cpuArchUpdate)(virCPUDefPtr guest,
  const virCPUDefPtr host);
 
+typedef bool
+(*cpuArchHasFeature) (const union cpuData *data,
+  const char *feature);
+
 
 struct cpuArchDriver {
 const char *name;
@@ -95,6 +99,7 @@ struct cpuArchDriver {
 cpuArchGuestDataguestData;
 cpuArchBaseline baseline;
 cpuArchUpdate   update;
+cpuArchHasFeaturehasFeature;
 };
 
 
@@ -151,4 +156,10 @@ extern int
 cpuUpdate   (virCPUDefPtr guest,
  const virCPUDefPtr host);
 
+extern bool
+cpuHasFeature(const char *arch,
+  const union cpuData *data,
+  const char *feature);
+
+
 #endif /* __VIR_CPU_H__ */
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 1937901..42349f0 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1754,6 +1754,34 @@ cleanup:
 return ret;
 }
 
+static bool x86HasFeature(const union cpuData *data,
+  const char *name)
+{
+struct x86_map *map;
+struct x86_feature *feature;
+bool ret = false;
+int i;
+
+if (!(map = x86LoadMap()))
+return false;
+
+if (!(feature = x86FeatureFind(map, name)))
+goto cleanup;
+
+for (i = 0 ; i  feature-ncpuid ; i++) {
+struct cpuX86cpuid *cpuid;
+
+cpuid = x86DataCpuid(data, feature-cpuid[i].function);
+if (cpuid  x86cpuidMatchMasked(cpuid, feature-cpuid + i)) {
+ret = true;
+goto cleanup;
+}
+}
+
+cleanup:
+x86MapFree(map);
+return ret;
+}
 
 struct cpuArchDriver cpuDriverX86 = {
 .name = x86,
@@ -1771,4 +1799,5 @@ struct cpuArchDriver cpuDriverX86 = {
 .guestData  = x86GuestData,
 .baseline   = x86Baseline,
 .update = x86Update,
+.hasFeature = x86HasFeature,
 };
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 301b0ef..0189183 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -90,6 +90,7 @@ cpuEncode;
 cpuGuestData;
 cpuNodeData;
 cpuUpdate;
+cpuHasFeature;
 
 
 # cpu_conf.h
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 737b255..429c399 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1210,6 +1210,8 @@ static unsigned long long qemudComputeCmdFlags(const char 
*help,
 flags |= QEMUD_CMD_FLAG_NO_KVM_PIT;
 if (strstr(help, -tdf))
 flags |= QEMUD_CMD_FLAG_TDF;
+if (strstr(help, -enable-nesting))
+flags |= QEMUD_CMD_FLAG_NESTING;
 if (strstr(help, ,menu=on))
 flags |= QEMUD_CMD_FLAG_BOOT_MENU;
 
@@ -3503,7 +3505,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
const char *emulator,
unsigned long long qemuCmdFlags,
const struct utsname *ut,
-   char **opt)
+   char **opt,

Re: [libvirt] [PATCH v4 01/13] Adding structure and defines for virDomainSet/GetMemoryParameters

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:44:48PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 This patch adds a structure virMemoryParameter, it contains the name of the
 parameter and the type of the parameter along with a union.
 
 v4:
 + Add unsigned int flags to the public api for future extensions
 
 v3:
 + Protoype for virDomainGetMemoryParameters and dummy python binding.
 
 v2:
 + Includes dummy python bindings for the library to build cleanly.
 + Define string constants like hard_limit, etc.
 + re-order this patch.

  Okay, looks fine except :

 +/**
 + * virDomainMemoryParameterType:
 + *
 + * A memory parameter field type
 + */
 +typedef enum {
 +VIR_DOMAIN_MEMORY_FIELD_INT = 1, /* integer case */
 +VIR_DOMAIN_MEMORY_FIELD_UINT= 2, /* unsigned integer case */
 +VIR_DOMAIN_MEMORY_FIELD_LLONG   = 3, /* long long case */
 +VIR_DOMAIN_MEMORY_FIELD_ULLONG  = 4, /* unsigned long long case */
 +VIR_DOMAIN_MEMORY_FIELD_DOUBLE  = 5, /* double case */
 +VIR_DOMAIN_MEMORY_FIELD_BOOLEAN = 6  /* boolean(character) case */
 +} virMemoryParameterType;

  I'm renaming those to VIR_DOMAIN_MEMORY_PARAM_INT ... the 'field'
is not an important information, but the fact it's a memory parameter
type is the important point.

[...]
 +typedef virMemoryParameter *virMemoryParameterPtr;
 +
 +/* Set memory tunables for the domain*/
 +int virDomainSetMemoryParameters(virDomainPtr domain,
 +  virMemoryParameterPtr params,
 +  int nparams, unsigned int flags);
 +int virDomainGetMemoryParameters(virDomainPtr domain,
 +  virMemoryParameterPtr params,
 +  int *nparams, unsigned int flags);

I also removed tabs here, we don't use tabs, please use make syntax-check

  otherwise looks fine, ACK,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 04/13] XML parsing for memory tunables

[Balbir Singh]

* Nikunj A. Dadhania nik...@linux.vnet.ibm.com [2010-10-08 14:43:34]:

 On Fri, 8 Oct 2010 14:10:53 +0530, Balbir Singh bal...@linux.vnet.ibm.com 
 wrote:
  * Nikunj A. Dadhania nik...@linux.vnet.ibm.com [2010-10-08 12:00:44]:
  
   On Thu, 7 Oct 2010 12:49:29 +0100, Daniel P. Berrange 
   berra...@redhat.com wrote:
On Mon, Oct 04, 2010 at 12:47:22PM +0530, Nikunj A. Dadhania wrote:
 On Mon, 4 Oct 2010 12:16:42 +0530, Balbir Singh 
 bal...@linux.vnet.ibm.com wrote:
  * Nikunj A. Dadhania nik...@linux.vnet.ibm.com [2010-09-28 
  15:26:30]:
   snip
   +unsigned long hard_limit;
   +unsigned long soft_limit;
   +unsigned long min_guarantee;
   +unsigned long swap_hard_limit;
  
  The hard_limit, soft_limit, swap_hard_limit are s64 and the value is
  in bytes. What is the unit supported in this implementation?

Actually if libvirt is built on 32bit these aren't big enough - make
them into 'unsigned long long' data types I reckon.

   I was thinking that as we are having the unit of KB, we would be able to
   represent 2^42 bytes of memory limit, ie. 4 Terabytes. Won't this suffice 
   in
   case of 32bit?
  
  
  How would you represent -1 (2^63 -1) as unlimited or max limit we use
  today? 
  
 I think I have answered this question in the thread: this is specific to
 cgroup that -1 means unlimited, this may not be true for other HVs.

OK, so how do we handle unlimited values in general?

-- 
Three Cheers,
Balbir

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 02/13] Adding virDomainSetMemoryParameters and virDomainGetMemoryParameters API

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:45:00PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 Public api to set/get memory tunables supported by the hypervisors.
 RFC: https://www.redhat.com/archives/libvir-list/2010-August/msg00607.html
 
 v4:
 * Move exporting public API to this patch
 * Add unsigned int flags to the public api for future extensions
 
 v3:
 * Add domainGetMemoryParamters and NULL in all the driver interface
 
 v2:
 * Initialize domainSetMemoryParameters to NULL in all the driver interface
   structure.
 
[...]
 --- a/src/libvirt.c
 +++ b/src/libvirt.c
 @@ -3000,6 +3000,110 @@ error:
  }
  
  /**
 + * virDomainSetMemoryParameters:
 + * @domain: pointer to domain object
 + * @params: pointer to memory parameter objects
 + * @nparams: number of memory parameter (this value should be same or
 + *  less than the number of parameters supported)
 + * @flags: currently unused, for future extension
 + *
 + * Change the memory tunables
 + * This function requires privileged access to the hypervisor. 
 + *
 + * Returns -1 in case of error, 0 in case of success.
 + */
 +int 
 +virDomainSetMemoryParameters(virDomainPtr domain,
 +  virMemoryParameterPtr params,
 +  int nparams, unsigned int flags)

  I had to remove tabs and trailing spaces at end of lines here.

 +{
 +virConnectPtr conn;
 +DEBUG(domain=%p, params=%p, nparams=%d, flags=%u, domain, params, 
 nparams, flags);
 +
 +virResetLastError();
 +
 +if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
 +virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
 +virDispatchError(NULL);
 +return -1;
 +}
 +if (domain-conn-flags  VIR_CONNECT_RO) {
 +virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
 +goto error;
 +}

Seems that params = 0 or a NULL params are errors in this function
based on the API description, so I prefer to catch those here and added

if ((nparams = 0) || (params == NULL)) {
virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}

 +conn = domain-conn;
 +
 +if (conn-driver-domainSetMemoryParameters) {
[...]
 +/**
 + * virDomainGetMemoryParameters:
 + * @domain: pointer to domain object
 + * @params: pointer to memory parameter object
 + *  (return value, allocated by the caller)
 + * @nparams: pointer to number of memory parameters
 + * @flags: currently unused, for future extension
 + *
 + * Get the memory parameters, the @params array will be filled with the 
 values
 + * equal to the number of parameters suggested by @nparams
 + *
 + * As a special case, if @nparams is zero and @params is NULL, the API will
 + * set the number of parameters supported by the HV in @nparams and return
 + * SUCCESS. 
 + *
 + * This function requires privileged access to the hypervisor. This function
 + * expects the caller to allocate the 

  unterminated comment. I fixed this as
* expects the caller to allocate the @param

 + * Returns -1 in case of error, 0 in case of success.
 + */
 +int 
 +virDomainGetMemoryParameters(virDomainPtr domain,
 +  virMemoryParameterPtr params,
 +  int *nparams, unsigned int flags)
 +{

  same tabs and trailing spaces problems

 +virConnectPtr conn;
 +DEBUG(domain=%p, params=%p, nparams=%d, flags=%u, domain, params, 
 (nparams)?*nparams:-1, flags);
 +
 +virResetLastError();
 +
 +if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
 +virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
 +virDispatchError(NULL);
 +return -1;
 +}
 +if (domain-conn-flags  VIR_CONNECT_RO) {
 +virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
 +goto error;
 +}

  in that case params can be NULL, but nparams must not, and we can't
have *nparams  0 strictly so I'm adding:

if ((nparams == NULL) || (*nparams  0)) {
virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}

 +conn = domain-conn;

  That done, patch is fine,

ACK,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 02/13] Adding virDomainSetMemoryParameters and virDomainGetMemoryParameters API

[Eric Blake]

On 10/12/2010 08:01 AM, Daniel Veillard wrote:


Seems that params= 0 or a NULL params are errors in this function
based on the API description, so I prefer to catch those here and added

 if ((nparams= 0) || (params == NULL)) {
 virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
 goto error;
 }


Or even one step further, and use annotations to mark the function 
arguments as ATTRIBUTE_NONNULL to get compiler checking of your logic.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Enable support for nested SVM

[Eric Blake]

On 10/12/2010 04:46 AM, Daniel P. Berrange wrote:

This enables support for nested SVM using the regular CPU
model/features block. If the CPU model or features include
'svm', then the '-enable-nesting' flag will be added to the
QEMU command line. Latest out of tree patches for nested
'vmx', no longer require the '-enable-nesting' flag. They
instead just look at the cpu features. Several of the models
already include svm support, but QEMU was just masking out
the svm bit silently. So this will enable SVM on such
models

+
+bool
+cpuHasFeature(const char *arch,
+  const union cpuData *data,
+  const char *feature)
+{
+struct cpuArchDriver *driver;
+
+VIR_DEBUG(arch=%s, data=%p, feature=%s,
+  arch, data, feature);
+
+if ((driver = cpuGetSubDriver(arch)) == NULL)
+return -1;


Ouch.  -1 promotes to true.


+
+if (driver-hasFeature == NULL) {
+virCPUReportError(VIR_ERR_NO_SUPPORT,
+_(cannot check guest CPU data for %s architecture),
+  arch);
+return -1;


Likewise.


+}
+
+return driver-hasFeature(data, feature);
+}


You either need to change the return type to int and take a bool* 
parameter (return -1 on failure, 0 on success when *param was written 
to), or return an int value rather than a bool; since the caller needs 
to distinguish between feature-not-present and error-message-reported.



+
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index a745917..3a7b996 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -82,6 +82,10 @@ typedef int
  (*cpuArchUpdate)(virCPUDefPtr guest,
   const virCPUDefPtr host);

+typedef bool
+(*cpuArchHasFeature) (const union cpuData *data,
+  const char *feature);


But this typedef is okay.

...

+for (i = 0 ; i  feature-ncpuid ; i++) {
+struct cpuX86cpuid *cpuid;
+
+cpuid = x86DataCpuid(data, feature-cpuid[i].function);
+if (cpuid  x86cpuidMatchMasked(cpuid, feature-cpuid + i)) {
+ret = true;
+goto cleanup;


I probably would have used 'break' rather than 'goto cleanup', since 
it's shorter, but since you already have to have the label due to 
earlier code in the method, either way is fine.



+}
+}
+
+cleanup:
+x86MapFree(map);
+return ret;
+}




+++ b/src/qemu/qemu_conf.c
@@ -1210,6 +1210,8 @@ static unsigned long long qemudComputeCmdFlags(const char 
*help,
  flags |= QEMUD_CMD_FLAG_NO_KVM_PIT;
  if (strstr(help, -tdf))
  flags |= QEMUD_CMD_FLAG_TDF;
+if (strstr(help, -enable-nesting))
+flags |= QEMUD_CMD_FLAG_NESTING;
  if (strstr(help, ,menu=on))
  flags |= QEMUD_CMD_FLAG_BOOT_MENU;



Any reason you didn't put the new feature at the end of the list, in 
enum order?



@@ -3555,6 +3560,12 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
  if (cpuDecode(guest, data, cpus, ncpus, preferred)  0)
  goto cleanup;

+/* Only 'svm' requires --enable-nesting. The out-of-tree
+ * 'vmx' patches now simply hook off the CPU features


This comment will be out-of-date once the vmx patches are no longer out 
of tree.  Should we just say:


Nested vmx support in qemu simply hooks off the CPU features

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: add missing export

[Matthias Bolte]

2010/10/12 Eric Blake ebl...@redhat.com:
 Commit 1fe2927a3 forgot to export a symbol.

 * src/libvirt_private.syms (virHexToBin): Add.
 * src/.gitignore: Ignore temporary file.
 ---
  src/.gitignore           |    1 +
  src/libvirt_private.syms |    1 +
  2 files changed, 2 insertions(+), 0 deletions(-)

 diff --git a/src/.gitignore b/src/.gitignore
 index 7ea8d89..a619643 100644
 --- a/src/.gitignore
 +++ b/src/.gitignore
 @@ -14,4 +14,5 @@ libvirt.syms
  libvirt_qemu.def
  *.i
  *.s
 +remote_protocol-structs-t
  virt-aa-helper
 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 index 301b0ef..1d94b12 100644
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -668,6 +668,7 @@ virFileResolveLink;
  saferead;
  safewrite;
  safezero;
 +virHexToBin;
  virMacAddrCompare;
  virEnumFromString;
  virEnumToString;
 --
 1.7.2.3


ACK.

How did you notice this?

Matthias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: add missing export

[Eric Blake]

On 10/12/2010 08:23 AM, Matthias Bolte wrote:

--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -668,6 +668,7 @@ virFileResolveLink;
  saferead;
  safewrite;
  safezero;
+virHexToBin;
  virMacAddrCompare;
  virEnumFromString;
  virEnumToString;
--
1.7.2.3



ACK.


Thanks; pushing shortly.



How did you notice this?


For my vcpu patches, I was about to add virCountOneBits in util.c, and 
looked at how previous additions had done things.  Then it hit me - we 
already have gnulib's count-one-bits.h, so I scrapped my new API, but 
kept the bug fix I had found on the way.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/12] vcpu: add virsh support

[Eric Blake]

On 09/29/2010 06:02 PM, Eric Blake wrote:

* tools/virsh.c (cmdSetvcpus): Add new flags.  Let invalid
commands through to driver, to ease testing of hypervisor argument
validation.
(cmdVcpucount): New command.
(commands): Add new command.
* tools/virsh.pod (setvcpus, vcpucount): Document new behavior.
---


In testing this, I found it useful to add one more command.

Previously, virsh had no way to get at 
virConnectGetMaxVcpus/virDomainGetMaxVcpus (it is used it inside 
setvcpus, but not exposed to the user).  And now that 
virDomainGetVcpusFlags is smarter about reading the maximum limit 
associated with the XML of a domain, it is harder to tell whether the 
maximum associated with a domain is due to the domain's xml or due to 
the XML omitting the vcpus element and inheriting the hypervisor's 
maximum.  That is, with more flexibility in vcpu management, it is also 
more important to know, for example, that the max vcpus permitted by xen 
is 32, and the max vcpus permitted by qemu is dependent on the number of 
cores in the host, whether or not a given domain is using that many vcpus.


I debated between two interfaces:

1. Make vcpucount smarter:
vcpucount = virConnectGetMaxVcpus, plus table of all vcpu stats on all 
domains

vcpucount domain = table of all vcpu stats on domain
vcpucount {--live|--config} {--curent|--maximum} domain = single stat
vcpucount domain... = table of all vcpu stats on listed domains

2. Add second command, leaving vcpucount unchanged from v1:
maxvcpus [--type=string] = virConnectGetMaxVcpus

then decided to go with option 2 in my v2 posting of the vcpu series, 
unless anyone has a reason why overloading a single command makes more 
sense than keeping the separate API calls under separate commands.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 02/13] Adding virDomainSetMemoryParameters and virDomainGetMemoryParameters API

[Daniel P. Berrange]

On Tue, Oct 12, 2010 at 08:16:11AM -0600, Eric Blake wrote:
 On 10/12/2010 08:01 AM, Daniel Veillard wrote:
 
 Seems that params= 0 or a NULL params are errors in this function
 based on the API description, so I prefer to catch those here and added
 
  if ((nparams= 0) || (params == NULL)) {
  virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
  goto error;
  }
 
 Or even one step further, and use annotations to mark the function 
 arguments as ATTRIBUTE_NONNULL to get compiler checking of your logic.

That's only true if the client application has the neccessary compile
time warnings enabled during their build. If you annotate a function
with nonnull, the docs say this activates further compiler optimizations.
So I'd be concerned that annotating the public APIs with  nonnull might
let the compiler optimize away that 'params == NULL' check to nothing.
At which point the app using libvirt would be at risk if they had not
enabled compile warnings to activate the annotation

Daniel
-- 
|: Red Hat, Engineering, London-o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org-o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 03/13] Adds xml entries for memory tunables

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:45:12PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 The patch adds xml entries to the domain.rng file.
 

  Except for spurious tabs, looks fine, ACK,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 04/13] XML parsing for memory tunables

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:45:23PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 Adding parsing code for memory tunables in the domain xml file
 
 v4:
 * Add memtune in tests/qemuxml2xmltest.c
 * Fix: insert memtune element only when any of them is set
 
 v2:
 + Fix typo min_guarantee

  The patch is fine except the usual space and tabs mixups and the fact
that a number of drivers still needed to be converted to the change
of the definition structure. grep -- -memory src/*/* isn't that
hard and would have shown that even the driver for your own IBM Phyp
hardware failed to compile after your patch !!

  anyway once cleaned up the patch makes sensei, ACK, but please use
make syntax-check and do not configure out drivers when you are
developping patches,

  thanks,

Daniel


-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 05/13] Implement cgroup memory controller tunables

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:45:34PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 Provides interfaces for setting/getting memory tunables like hard_limit,
 soft_limit and swap_hard_limit

  ACK, just one extra misplaced space,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 06/13] Implement driver interface domainSetMemoryParamters for QEmu

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:45:44PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 Driver interface for setting memory hard_limit, soft_limit and swap
 hard_limit.
[...]
 +static int qemuDomainSetMemoryParameters(virDomainPtr dom,
 + virMemoryParameterPtr params,
 + int nparams, 
 + unsigned int flags ATTRIBUTE_UNUSED)
 +{
 +struct qemud_driver *driver = dom-conn-privateData;
 +int i;
 +virCgroupPtr group = NULL;
 +virDomainObjPtr vm = NULL;
 +int ret = -1;
 +
 +qemuDriverLock(driver);
 +if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) {
 +qemuReportError(VIR_ERR_NO_SUPPORT,
 +__FUNCTION__);
 +goto cleanup;
 +}
 +
 +vm = virDomainFindByUUID(driver-domains, dom-uuid);
 +
 +if (vm == NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +_(No such domain %s), dom-uuid);
 +goto cleanup;
 +}
 +
 +if (virCgroupForDomain(driver-cgroup, vm-def-name, group, 0) != 0) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +_(cannot find cgroup for domain %s), 
 vm-def-name);
 +goto cleanup;
 +}
 +
 +for (i = 0; i  nparams; i++) {
 +virMemoryParameterPtr param = params[i];

  Bingo if you passed a NULL pointer at the library entry point this
would just crash. Well you can still get this to crash with a value of
nparams bigger than the array. It's one of my main concern with this
API, it's a bit easy for the user to get things wrong, at least we
should provide minimal checkings.
  I still prefer to keep those checks in the top function in libvirt.c
to avoid duplicating in all drivers, but here we could check that
nparams is between 1 and 3 c.f. the comment on next patch:

+/* Current number of memory parameters supported by cgroups is
3
+ * FIXME: Magic number, need to see where should this go
+ */
+*nparams = 3;

  that 3 need to be abstracted somehow as

#define QEMU_NB_MEM_PARAM  3

  which I'm adding at the beginning of the file.
However if we check that nparams  3, we will introduce an
incompatibility, for example if we try to set tunables from a newver
library version but talking to an older server not implementing the
new ones, we return an error, but ...

[...]
 +} else if (STREQ(param-field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE)) {
 +qemuReportError(VIR_ERR_INVALID_ARG,
 +_(Memory tunable `%s' not implemented), 
 param-field);
 +} else {
 +qemuReportError(VIR_ERR_INVALID_ARG,
 +_(Parameter `%s' not supported), param-field);
 +}

  right now the code just ignores if you failed to set a tunable.
I think this is a problem, we should at least return an error if a
tunable could not be set. Right now the error would be raised within
libvirt daemon but since there is no error code the application may
not get the problem. So in those 2 cases I suggest to set an error.
So I'm changing this to set ret to 0 before the loop and setting
ret = -1; in any of the error cases found within.

 +}
 +ret = 0;

  moved in front of loop.
Quite a bit of change but once done I commited it to my tree

Daniel


-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] cpu: Remove redundant features

[Eric Blake]

On 10/12/2010 04:15 AM, Jiri Denemark wrote:

Some features provided by the recently added CPU models were mentioned
twice for each model. This was a result of automatic generation of the
XML from qemu's CPU configuration file without noticing this redundancy.
---
  src/cpu/cpu_map.xml |   84 ---
  1 files changed, 0 insertions(+), 84 deletions(-)


ACK. Patches of pure deletion are always fun :)

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 0/8] virsh: rework command parsing

[Eric Blake]

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:



Old virsh command parsing mashes all the args back into a string and
miss the quotes, this patches fix it. It is also needed for introducing
qemu-monitor-command which is very useful.

This patches add vrshCommandParser abstraction to parser the args.


Thanks for splitting it up!  It will make reviews easier.  However, in 
the future, you should try and convince 'git send-email' or whatever 
mechanism you use to send patches to do shallow threading (all 8 of the 
n/8 patches should be in-reply-to the 0/8 cover letter), as it makes 
reviewing easier when the series is a single thread.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 07/13] Implement driver interface domainGetMemoryParamters for QEmu

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:45:55PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 V4:
 * prototype change: add unsigned int flags
 
 Driver interface for getting memory parameters, eg. hard_limit, soft_limit and
 swap_hard_limit.
 
 Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 ---
  src/qemu/qemu_driver.c |  120 
 
  1 files changed, 119 insertions(+), 1 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 471db39..8eaa762 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -9458,6 +9458,124 @@ cleanup:
  return ret;
  }

  same things about crashing if the arguments are invalid

 +if ((*nparams) == 0) {
 +/* Current number of memory parameters supported by cgroups is 3
 + * FIXME: Magic number, need to see where should this go
 + */
 +*nparams = 3;
 +ret = 0;
 +goto cleanup;
 +}
 +
 +if ((*nparams) != 3) {

  using QEMU_NB_MEM_PARAM instead of the raw 3 value c.f. previous patch
  comment

 +qemuReportError(VIR_ERR_INVALID_ARG,
 +%s, _(Invalid parameter count));
 +goto cleanup;
 +}

  okay, this mean the application must always call with 0 first to get
  the exact value or this will break, fine but probably need to be made
  more clear from the description in libvirt.c  TODO

 +if (virCgroupForDomain(driver-cgroup, vm-def-name, group, 0) != 0) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +_(cannot find cgroup for domain %s), 
 vm-def-name);
 +goto cleanup;
 +}
 +
 +for (i = 0; i  *nparams; i++) {
 +virMemoryParameterPtr param = params[i];
 +val = 0;
 +param-value.ul = 0;
 +param-type = VIR_DOMAIN_MEMORY_FIELD_ULLONG;
 +
 +switch(i) {
 +case 0: /* fill memory hard limit here */
 +rc = virCgroupGetMemoryHardLimit(group, val);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to get memory hard limit));
 +continue;
 +}
 +if (virStrcpyStatic(param-field, VIR_DOMAIN_MEMORY_HARD_LIMIT) 
 == NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +%s, _(Field memory hard limit too long 
 for destination));
 +continue;
 +}
 +param-value.ul = val;
 +break;
 +
 +case 1: /* fill memory soft limit here */
 +rc = virCgroupGetMemorySoftLimit(group, val);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to get memory soft limit));
 +continue;
 +}
 +if (virStrcpyStatic(param-field, VIR_DOMAIN_MEMORY_SOFT_LIMIT) 
 == NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +%s, _(Field memory soft limit too long 
 for destination));
 +continue;
 +}
 +param-value.ul = val;
 +break;
 +   
 +case 2: /* fill swap hard limit here */
 +rc = virCgroupGetSwapHardLimit(group, val);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to get swap hard limit));
 +continue;
 +}
 +if (virStrcpyStatic(param-field, VIR_DOMAIN_SWAP_HARD_LIMIT) == 
 NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +%s, _(Field swap hard limit too long for 
 destination));
 +continue;
 +}
 +param-value.ul = val;
 +break;
 +
 +default:
 +break;
 +/* should not hit here */
 +}
 +}

  Okay, I'm not sure we actually need a loop here, but it may help
  refactoring...
I'm still having a problem with the code ignoring any error occuring in
the loop, and fixing this in the same way. If there is an error the
application *must* learn about it instead of trusting uninitialized
memory as being data !
Maybe a memset is in order actually before entering that loop to avoid
edge case problems... TODO too

 +ret = 0;
 +
 +cleanup:
 +if (group)
 +virCgroupFree(group);
 +if (vm)
 +virDomainObjUnlock(vm);
 +qemuDriverUnlock(driver);
 +return ret;
 +}
 +
  static int qemuSetSchedulerParameters(virDomainPtr dom,
virSchedParameterPtr params,
int nparams)
 @@ -12804,7 +12922,7 @@ static virDriver qemuDriver = {
  qemuDomainSnapshotDelete, /* domainSnapshotDelete */
  qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */
  

Re: [libvirt] [PATCH] cpu: Remove redundant features

[Jiri Denemark]

  Some features provided by the recently added CPU models were mentioned
  twice for each model. This was a result of automatic generation of the
  XML from qemu's CPU configuration file without noticing this redundancy.
  ---
src/cpu/cpu_map.xml |   84 
  ---
1 files changed, 0 insertions(+), 84 deletions(-)
 
 ACK. Patches of pure deletion are always fun :)

Thanks and don't worry, better fun is coming soon (a patchset with about 1000
insertions and 1 deletion) :-)

I pushed this patch.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 08/13] Adding memtunables to qemuSetupCgroup

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:46:05PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 v4:
 * Fix: call cgroup apis only if tunables are non zero
 
 QEmu startup would pick up the memory tunables specified in the domain
 configuration file.
 
 Acked-by: Daniel P. Berrange berra...@redhat.com
 Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com

  ACK,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 0/8] virsh: rework command parsing

[Eric Blake]

On 10/12/2010 09:50 AM, Eric Blake wrote:

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:



Old virsh command parsing mashes all the args back into a string and
miss the quotes, this patches fix it. It is also needed for introducing
qemu-monitor-command which is very useful.

This patches add vrshCommandParser abstraction to parser the args.


Thanks for splitting it up! It will make reviews easier. However, in the
future, you should try and convince 'git send-email' or whatever
mechanism you use to send patches to do shallow threading (all 8 of the
n/8 patches should be in-reply-to the 0/8 cover letter), as it makes
reviewing easier when the series is a single thread.


Now that I've done a bit more research; here's the results, to make it 
easier for others to do:


git config sendemail.chainreplyto false
git config sendemail.thread false
git config format.thread shallow

Then:

git send-email -8 --cover-letter

will automatically send 8 messages all in reply to the cover letter, 
creating a single thread.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 10/13] Implement driver interface domainSetMemoryParamters for LXC

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:46:28PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 Add support in the lxc driver for various memory controllable parameters
 
 v4:
 + prototype change: add unsigned int flags
 
 v2:
 + Use #define string constants for hard_limit, etc
 + fix typo: min_guarantee
 
 Acked-by: Daniel P. Berrange berra...@redhat.com
 Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
[...]
 +static int lxcDomainSetMemoryParameters(virDomainPtr dom,
 +virMemoryParameterPtr params,
 +int nparams, 
 +unsigned int flags ATTRIBUTE_UNUSED)
 +{
 +lxc_driver_t *driver = dom-conn-privateData;
 +int i;
 +virCgroupPtr cgroup = NULL;
 +virDomainObjPtr vm = NULL;
 +int ret = -1;
 +
 +lxcDriverLock(driver);
 +vm = virDomainFindByUUID(driver-domains, dom-uuid);
 +
 +if (vm == NULL) {
 +char uuidstr[VIR_UUID_STRING_BUFLEN];
 +virUUIDFormat(dom-uuid, uuidstr);
 +lxcError(VIR_ERR_NO_DOMAIN,
 + _(No domain with matching uuid '%s'), uuidstr);
 +goto cleanup;
 +}

  Hum, the qemu driver was reporting

if (vm == NULL) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
_(No such domain %s), dom-uuid);
goto cleanup;
}

the 2 should be harmonized I guess, but since the LXC reporting is better
I left this as a TODO, seems that's a more general cleanup needed between
drivers.

 +if (virCgroupForDomain(driver-cgroup, vm-def-name, cgroup, 0) != 0) {
 +lxcError(VIR_ERR_INTERNAL_ERROR,
 + _(Unable to get cgroup for %s), vm-def-name);
 +goto cleanup;
 +}

And QEmu reported here:

qemuReportError(VIR_ERR_INTERNAL_ERROR,
_(cannot find cgroup for domain %s), vm-def-name);
goto cleanup;

why diverging strings ? ... I used the same string instead !

 +for (i = 0; i  nparams; i++) {
 +virMemoryParameterPtr param = params[i];
 +
 +if (STREQ(param-field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) {
 +int rc;
 +if (param-type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
 +lxcError(VIR_ERR_INVALID_ARG, %s,
 + _(invalid type for memory hard_limit tunable, 
 expected a 'ullong'));
 +continue;
 +}
 +
 +rc = virCgroupSetMemoryHardLimit(cgroup, params[i].value.ul);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to set memory hard_limit 
 tunable));
 +}
 +} else if (STREQ(param-field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) {
 +int rc;
 +if (param-type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
 +lxcError(VIR_ERR_INVALID_ARG, %s,
 + _(invalid type for memory soft_limit tunable, 
 expected a 'ullong'));
 +continue;
 +}
 +
 +rc = virCgroupSetMemorySoftLimit(cgroup, params[i].value.ul);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to set memory soft_limit 
 tunable));
 +}
 +} else if (STREQ(param-field, VIR_DOMAIN_SWAP_HARD_LIMIT)) {
 +int rc;
 +if (param-type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
 +lxcError(VIR_ERR_INVALID_ARG, %s,
 + _(invalid type for swap_hard_limit tunable, 
 expected a 'ullong'));
 +continue;
 +}
 +
 +rc = virCgroupSetSwapHardLimit(cgroup, params[i].value.ul);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to set swap_hard_limit 
 tunable));
 +}
 +} else if (STREQ(param-field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE)) {
 +lxcError(VIR_ERR_INVALID_ARG,
 + _(Memory tunable `%s' not implemented), param-field);
 +} else {
 +lxcError(VIR_ERR_INVALID_ARG,
 + _(Parameter `%s' not supported), param-field);
 +}
 +}
 +ret = 0;

  Same problem of error reporting as in the QEmu driver, I moved ret = 0;
before the loop and et ret = -1; on all errors !

 +cleanup:
 +if (cgroup)
 +virCgroupFree(cgroup);
 +if (vm)
 +virDomainObjUnlock(vm);
 +lxcDriverUnlock(driver);
 +return ret;
 +}
 +

 After those modifications, ACK, applied to my tree,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 11/13] Implement driver interface domainGetMemoryParamters for LXC

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:46:40PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 v4:
 * prototype change: add unsigned int flags
 
 Driver interface for getting memory parameters, eg. hard_limit, soft_limit and
 swap_hard_limit.
 
 Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 ---
  src/lxc/lxc_driver.c |  114 
 ++
  1 files changed, 113 insertions(+), 1 deletions(-)
 
 diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
 index 984a5fa..036dedf 100644
 --- a/src/lxc/lxc_driver.c
 +++ b/src/lxc/lxc_driver.c
 @@ -766,6 +766,118 @@ cleanup:
  return ret;
  }

 same as for the QEmu driver equivalent, I fixed the error handling and
create a new constant at the top of the file.

 Once done, ACK, and pushed to my tree,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/8] virsh: better support double quote

[Eric Blake]

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:


In origin code, double quote is only allowed at the begin or end
complicated argument
--some_opt=complicated string  (we split this argument into 2 parts,
option and data, the data is complicated string).

This patch makes it allow double quote at any position of
an argument:
complicated argument
complicated argument
--some opt=complicated string

This patch is also needed for the following patches,
the following patches will not split option argument into 2 parts,
so we have to allow double quote at any position of an argument.

Signed-off-by: Lai Jiangshanla...@cn.fujitsu.com


I had to add your name to AUTHORS to make syntax-check happy.


---
diff --git a/tools/virsh.c b/tools/virsh.c
index 57ea618..7b6f2b6 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10172,9 +10172,9 @@ static int
  vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
  {
  int tk = VSH_TK_NONE;
-int quote = FALSE;
+bool double_quote = false;
  int sz = 0;
-char *p = str;
+char *p = str, *q;


Maybe it's just me, but I tend to declare multiple pointers on separate 
lines.  But no big deal.



-} else if (quote  *p == '') {
-quote = FALSE;
+}
+
+   if (*p == '') {


'make syntax-check' would have caught this TAB.


+double_quote = !double_quote;
  p++;
-break;  /* end of ... token */
+continue;
  }
+
+if (*res)
+(*res)[sz] = *p;


That's a lot of indirection, for every byte of the loop.  Wouldn't it be 
better to have a local temporary, and only assign to *res at the end?



  p++;
  sz++;
  }
-if (quote) {
+if (double_quote) {
  vshError(ctl, %s, _(missing \));
  return VSH_TK_ERROR;
  }
@@ -10231,8 +10234,12 @@ vshCommandGetToken(vshControl *ctl, char *str, char 
**end, char **res)
  if (sz == 0)
  return VSH_TK_END;

-*res = vshMalloc(ctl, sz + 1);
-memcpy(*res, tkstr, sz);
+if (!*res) {
+*res = vshMalloc(ctl, sz + 1);
+sz = 0;
+p = q;
+goto copy;
+}


Hmm, a backwards jump, which means we parse every string twice - once to 
figure out how long it is, and again to strip quotes.  Yes, that avoids 
over-allocating, but are we really that tight on memory?  I find it 
quicker to just strdup() up front, then edit in-place on a single pass.


Hmm, one thing you _don't_ recognize is:

--option

as an option.  In the shell, quotes are stripped before arguments are 
recognized as a particular token.  I think that's pretty easy to support 
- delay VSH_TK_OPTION checking until after we've stripped quotes, but 
I'll show it as a separate patch.


Hmm, I also noticed that we're inconsistent on strdup/vshStrdup in this 
file; separate cleanup patch for that coming up soon.


But, with those nits fixed, ACK.  Here's what I'm squashing in to my 
local tree; I'll push it once I complete my review of your other 7 
patches, as well as getting approval to my promised followup patches.


diff --git i/AUTHORS w/AUTHORS
index 09169f2..a8f96df 100644
--- i/AUTHORS
+++ w/AUTHORS
@@ -129,6 +129,7 @@ Patches have also been contributed by:
diff --git i/tools/virsh.c w/tools/virsh.c
index 0a28c99..4f70724 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10124,16 +10124,18 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
 #define VSH_TK_DATA2
 #define VSH_TK_END3

-static int
+static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
 vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
 {
 int tk = VSH_TK_NONE;
 bool double_quote = false;
 int sz = 0;
-char *p = str, *q;
+char *p = str;
+char *q = vshStrdup(ctl, str);
 char *tkstr = NULL;

 *end = NULL;
+*res = q;

 while (p  *p  (*p == ' ' || *p == '\t'))
 p++;
@@ -10145,9 +10147,6 @@ vshCommandGetToken(vshControl *ctl, char *str, 
char **end, char **res)

 return VSH_TK_END;
 }

-q = p;
-*res = NULL;
-copy:
 while (*p) {
 /* end of token is blank space or ';' */
 if (!double_quote  (*p == ' ' || *p == '\t' || *p == ';'))
@@ -10170,34 +10169,23 @@ copy:
 tkstr = p;  /* begin of token */
 }

-   if (*p == '') {
+if (*p == '') {
 double_quote = !double_quote;
 p++;
 continue;
 }

-if (*res)
-(*res)[sz] = *p;
-p++;
+*q++ = *p++;
 sz++;
 }
 if (double_quote) {
 vshError(ctl, %s, _(missing \));
 return VSH_TK_ERROR;
 }
-if (tkstr == NULL || *tkstr == '\0' || p == NULL)
-return VSH_TK_END;
-if (sz == 0)
+if (tkstr == NULL || *tkstr == '\0' || sz == 0)
 return VSH_TK_END;

-if (!*res) {
-*res = vshMalloc(ctl, sz + 1);
-sz = 0;
-p = q;
-goto copy;
-}
-*(*res + 

[libvirt] [PATCH] virsh: poison raw allocation routines

[Eric Blake]

* tools/virsh.c (malloc, calloc, realloc, strdup): Enforce that
within this file, we use the safe vsh wrappers instead.
(cmdNodeListDevices, cmdSnapshotCreate, main): Fix violations of
this policy.
---

 Hmm, I also noticed that we're inconsistent on strdup/vshStrdup in
 this file; separate cleanup patch for that coming up soon.

The bulk of this patch is code motion.

 tools/virsh.c |  117 ++--
 1 files changed, 63 insertions(+), 54 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 4f70724..5abf218 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -316,6 +316,66 @@ static void *_vshRealloc(vshControl *ctl, void *ptr, 
size_t sz, const char *file
 static char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, 
int line);
 #define vshStrdup(_ctl, _s)_vshStrdup(_ctl, _s, __FILE__, __LINE__)

+static void *
+_vshMalloc(vshControl *ctl, size_t size, const char *filename, int line)
+{
+void *x;
+
+if ((x = malloc(size)))
+return x;
+vshError(ctl, _(%s: %d: failed to allocate %d bytes),
+ filename, line, (int) size);
+exit(EXIT_FAILURE);
+}
+
+static void *
+_vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, 
int line)
+{
+void *x;
+
+if ((x = calloc(nmemb, size)))
+return x;
+vshError(ctl, _(%s: %d: failed to allocate %d bytes),
+ filename, line, (int) (size*nmemb));
+exit(EXIT_FAILURE);
+}
+
+static void *
+_vshRealloc(vshControl *ctl, void *ptr, size_t size, const char *filename, int 
line)
+{
+void *x;
+
+if ((x = realloc(ptr, size)))
+return x;
+VIR_FREE(ptr);
+vshError(ctl, _(%s: %d: failed to allocate %d bytes),
+ filename, line, (int) size);
+exit(EXIT_FAILURE);
+}
+
+static char *
+_vshStrdup(vshControl *ctl, const char *s, const char *filename, int line)
+{
+char *x;
+
+if (s == NULL)
+return(NULL);
+if ((x = strdup(s)))
+return x;
+vshError(ctl, _(%s: %d: failed to allocate %lu bytes),
+ filename, line, (unsigned long)strlen(s));
+exit(EXIT_FAILURE);
+}
+
+/* Poison the raw allocating identifiers in favor of our vsh variants.  */
+#undef malloc
+#undef calloc
+#undef realloc
+#undef strdup
+#define malloc use_vshMalloc_instead_of_malloc
+#define calloc use_vshCalloc_instead_of_calloc
+#define realloc use_vshRealloc_instead_of_realloc
+#define strdup use_vshStrdup_instead_of_strdup

 static int idsorter(const void *a, const void *b) {
   const int *ia = (const int *)a;
@@ -7253,7 +7313,7 @@ cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl-conn, 
devices[i]);
 if (dev  STRNEQ(devices[i], computer)) {
 const char *parent = virNodeDeviceGetParent(dev);
-parents[i] = parent ? strdup(parent) : NULL;
+parents[i] = parent ? vshStrdup(ctl, parent) : NULL;
 } else {
 parents[i] = NULL;
 }
@@ -8897,7 +8957,7 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)

 from = vshCommandOptString(cmd, xmlfile, NULL);
 if (from == NULL)
-buffer = strdup(domainsnapshot/);
+buffer = vshStrdup(ctl, domainsnapshot/);
 else {
 if (virFileReadAll(from, VIRSH_MAX_XML_FILE, buffer)  0) {
 /* we have to report the error here because during cleanup
@@ -10442,57 +10502,6 @@ vshError(vshControl *ctl, const char *format, ...)
 fputc('\n', stderr);
 }

-static void *
-_vshMalloc(vshControl *ctl, size_t size, const char *filename, int line)
-{
-void *x;
-
-if ((x = malloc(size)))
-return x;
-vshError(ctl, _(%s: %d: failed to allocate %d bytes),
- filename, line, (int) size);
-exit(EXIT_FAILURE);
-}
-
-static void *
-_vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, 
int line)
-{
-void *x;
-
-if ((x = calloc(nmemb, size)))
-return x;
-vshError(ctl, _(%s: %d: failed to allocate %d bytes),
- filename, line, (int) (size*nmemb));
-exit(EXIT_FAILURE);
-}
-
-static void *
-_vshRealloc(vshControl *ctl, void *ptr, size_t size, const char *filename, int 
line)
-{
-void *x;
-
-if ((x = realloc(ptr, size)))
-return x;
-VIR_FREE(ptr);
-vshError(ctl, _(%s: %d: failed to allocate %d bytes),
- filename, line, (int) size);
-exit(EXIT_FAILURE);
-}
-
-static char *
-_vshStrdup(vshControl *ctl, const char *s, const char *filename, int line)
-{
-char *x;
-
-if (s == NULL)
-return(NULL);
-if ((x = strdup(s)))
-return x;
-vshError(ctl, _(%s: %d: failed to allocate %lu bytes),
- filename, line, (unsigned long)strlen(s));
-exit(EXIT_FAILURE);
-}
-
 /*
  * Initialize connection.
  */
@@ -11074,7 +11083,7 @@ main(int argc, char **argv)
 ctl-log_fd = -1;   /* 

[libvirt] [PATCH 0/4] Support auditing of guests

[Daniel P. Berrange]

This patch series introduces basic support for auditing of guest
operations. The auditing hooks are primarily done in the libvirtd
dispatch layer, because we want to hook all stateful drivers 
like QEMU, LXC, UML, etc. We don't want to audit the remote driver,
VMWare, XenAPI etc which are just plain RPC drivers.

There is an exception for auditing of the SELinux label assignment.
That has to be done right inside the sVirt code since the neccessary
info isn't available in the libvirtd dispatch layer.

This patch series focuses on lifecycle operations, but there are
quite alot of other things that are desirable to audit in the 
future, so further patches will likely follow.

The last patch is semi-related, it fixes up a major screwup in
the linking of the daemon that caused duplicated copies of the
code to be linked. This was exposed by the audit work. 

The patches are a combination of work by myself and Miloslav

NB, it should compile and run fine with any reasonably recent
audit package, but if you want correctly identified log messages
you need audit 2.0.5

Also, audit logs only appear if running libvirtd as root. Non
root users don't have permissions to generate audit logs.

Daniel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/4] Audit VM start/stop/suspend/resume

[Daniel P. Berrange]

From: Miloslav Trmač m...@redhat.com

Most operations are audited at the libvirtd level; auditing in
src/libvirt.c would result in two audit entries per operation (one in
the client, one in libvirtd).

The only exception is a domain stopping of its own will (e.g. because
the user clicks on shutdown inside the interface).  There can often be
no client connected at the time the domain stops, so libvirtd does not
have any virConnectPtr object on which to attach an event watch.  This
patch therefore adds auditing directly inside the qemu driver (other
drivers are not supported).
---
 daemon/remote.c|  135 
 src/qemu/qemu_driver.c |8 +++
 2 files changed, 133 insertions(+), 10 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 6b67678..30c9031 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -57,6 +57,8 @@
 #include memory.h
 #include util.h
 #include stream.h
+#include uuid.h
+#include virtaudit.h
 #include libvirt/libvirt-qemu.h
 
 #define VIR_FROM_THIS VIR_FROM_REMOTE
@@ -1213,6 +1215,8 @@ remoteDispatchDomainCreate (struct qemud_server *server 
ATTRIBUTE_UNUSED,
 void *ret ATTRIBUTE_UNUSED)
 {
 virDomainPtr dom;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+int r;
 
 dom = get_nonnull_domain (conn, args-dom);
 if (dom == NULL) {
@@ -1220,11 +1224,18 @@ remoteDispatchDomainCreate (struct qemud_server *server 
ATTRIBUTE_UNUSED,
 return -1;
 }
 
-if (virDomainCreate (dom) == -1) {
+r = virDomainCreate(dom);
+
+virUUIDFormat(dom-uuid, uuidstr);
+VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_CONTROL, r != -1,
+  op=start name=%s uuid=%s, dom-name, uuidstr);
+
+if (r == -1) {
 virDomainFree(dom);
 remoteDispatchConnError(rerr, conn);
 return -1;
 }
+
 virDomainFree(dom);
 return 0;
 }
@@ -1239,6 +1250,8 @@ remoteDispatchDomainCreateWithFlags (struct qemud_server 
*server ATTRIBUTE_UNUSE
  remote_domain_create_with_flags_ret *ret)
 {
 virDomainPtr dom;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+int r;
 
 dom = get_nonnull_domain (conn, args-dom);
 if (dom == NULL) {
@@ -1246,7 +1259,15 @@ remoteDispatchDomainCreateWithFlags (struct qemud_server 
*server ATTRIBUTE_UNUSE
 return -1;
 }
 
-if (virDomainCreateWithFlags (dom, args-flags) == -1) {
+r = virDomainCreateWithFlags(dom, args-flags);
+
+virUUIDFormat(dom-uuid, uuidstr);
+VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_CONTROL, r != -1,
+  op=%s name=%s uuid=%s,
+  (args-flags  VIR_DOMAIN_START_PAUSED) !=
+  0 ? start-paused : start, dom-name, uuidstr);
+
+if (r == -1) {
 virDomainFree(dom);
 remoteDispatchConnError(rerr, conn);
 return -1;
@@ -1267,13 +1288,20 @@ remoteDispatchDomainCreateXml (struct qemud_server 
*server ATTRIBUTE_UNUSED,
remote_domain_create_xml_ret *ret)
 {
 virDomainPtr dom;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
 
 dom = virDomainCreateXML (conn, args-xml_desc, args-flags);
 if (dom == NULL) {
+VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_CONTROL, 0,
+  op=start name=? uuid=?);
 remoteDispatchConnError(rerr, conn);
 return -1;
 }
 
+virUUIDFormat(dom-uuid, uuidstr);
+VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_CONTROL, 1, op=start name=%s uuid=%s,
+  dom-name, uuidstr);
+
 make_nonnull_domain (ret-dom, dom);
 virDomainFree(dom);
 
@@ -1313,6 +1341,8 @@ remoteDispatchDomainDestroy (struct qemud_server *server 
ATTRIBUTE_UNUSED,
  void *ret ATTRIBUTE_UNUSED)
 {
 virDomainPtr dom;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+int r;
 
 dom = get_nonnull_domain (conn, args-dom);
 if (dom == NULL) {
@@ -1320,7 +1350,13 @@ remoteDispatchDomainDestroy (struct qemud_server *server 
ATTRIBUTE_UNUSED,
 return -1;
 }
 
-if (virDomainDestroy (dom) == -1) {
+r = virDomainDestroy(dom);
+
+virUUIDFormat(dom-uuid, uuidstr);
+VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_CONTROL, r != -1,
+  op=stop name=%s uuid=%s, dom-name, uuidstr);
+
+if (r == -1) {
 virDomainFree(dom);
 remoteDispatchConnError(rerr, conn);
 return -1;
@@ -1778,6 +1814,8 @@ remoteDispatchDomainMigratePrepare (struct qemud_server 
*server ATTRIBUTE_UNUSED
 r = virDomainMigratePrepare (conn, cookie, cookielen,
  uri_in, uri_out,
  args-flags, dname, args-resource);
+/* This creates a VM, but we don't audit it until the migration succeeds
+   and the VM actually starts. */
 if (r == -1) {
 VIR_FREE(uri_out);
 remoteDispatchConnError(rerr, conn);
@@ -1810,7 +1848,7 @@ remoteDispatchDomainMigratePerform (struct qemud_server 
*server ATTRIBUTE_UNUSED
 {
 int r;
 

[libvirt] [PATCH 4/4] Fix symbol exports remove duplicated libvirt_util.la linkage

[Daniel P. Berrange]

From: Miloslav Trmač m...@redhat.com

The libvirt_util.la library was mistakenly linked into libvirtd
directly. Since libvirt_util.la is already linked to libvirt.so,
this resulted in libvirtd getting two copies of the code and
more critically 2 copies of static global variables.

Testing in turn exposed a issue with loadable modules. The
gnulib replacement functions are not exported to loadable
modules. Rather than trying to figure out the name sof all
gnulib functions  export them, just linkage all loadable
modules against libgnu.la statically.

* daemon/Makefile.am: Remove linkage of libvirt_util.la
  and libvirt_driver.la
* src/Makefile.am: Link driver modules against libgnu.la
* src/libvirt.c: Don't try to load modules which were
  compiled out
* src/libvirt_private.syms: Export all other internal
  symbols that are required  by drivers
---
 daemon/Makefile.am   |6 ++
 src/Makefile.am  |   24 +++-
 src/libvirt.c|   14 ++
 src/libvirt_private.syms |   45 +
 4 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index b020b77..987133c 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -99,11 +99,9 @@ libvirtd_LDADD = \
$(SASL_LIBS)\
$(POLKIT_LIBS)
 
-libvirtd_LDADD += ../src/libvirt_util.la ../src/libvirt-qemu.la
+libvirtd_LDADD += ../src/libvirt-qemu.la
 
-if WITH_DRIVER_MODULES
-  libvirtd_LDADD += ../src/libvirt_driver.la
-else
+if ! WITH_DRIVER_MODULES
 if WITH_QEMU
 libvirtd_LDADD += ../src/libvirt_driver_qemu.la
 endif
diff --git a/src/Makefile.am b/src/Makefile.am
index 8ec8230..521246c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -82,7 +82,7 @@ UTIL_SOURCES =
\
util/uuid.c util/uuid.h \
util/util.c util/util.h \
util/xml.c util/xml.h   \
-   util/virtaudit.c util/virtaudit.h   \
+   util/virtaudit.c util/virtaudit.h   \
util/virterror.c util/virterror_internal.h
 
 EXTRA_DIST += util/threads-pthread.c util/threads-win32.c
@@ -566,6 +566,7 @@ libvirt_driver_xen_la_CFLAGS =  
\
 libvirt_driver_xen_la_LDFLAGS = $(AM_LDFLAGS)
 libvirt_driver_xen_la_LIBADD = $(XEN_LIBS)
 if WITH_DRIVER_MODULES
+libvirt_driver_xen_la_LIBADD += ../gnulib/lib/libgnu.la
 libvirt_driver_xen_la_LDFLAGS += -module -avoid-version
 endif
 libvirt_driver_xen_la_SOURCES = $(XEN_DRIVER_SOURCES)
@@ -578,7 +579,8 @@ else
 noinst_LTLIBRARIES += libvirt_driver_phyp.la
 libvirt_la_BUILT_LIBADD += libvirt_driver_phyp.la
 endif
-libvirt_driver_phyp_la_LIBADD = $(LIBSSH2_LIBS)
+libvirt_driver_phyp_la_LIBADD = ../gnulib/lib/libgnu.la
+libvirt_driver_phyp_la_LIBADD += $(LIBSSH2_LIBS)
 libvirt_driver_phyp_la_CFLAGS = $(LIBSSH2_CFLAGS) \
-...@top_srcdir@/src/conf $(AM_CFLAGS)
 libvirt_driver_phyp_la_SOURCES = $(PHYP_DRIVER_SOURCES)
@@ -594,6 +596,7 @@ endif
 libvirt_driver_openvz_la_CFLAGS = \
-...@top_srcdir@/src/conf $(AM_CFLAGS)
 if WITH_DRIVER_MODULES
+libvirt_driver_openvz_la_LIBADD = ../gnulib/lib/libgnu.la
 libvirt_driver_openvz_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 endif
 libvirt_driver_openvz_la_SOURCES = $(OPENVZ_DRIVER_SOURCES)
@@ -608,10 +611,11 @@ libvirt_la_BUILT_LIBADD += libvirt_driver_vbox.la
 endif
 libvirt_driver_vbox_la_CFLAGS = \
-...@top_srcdir@/src/conf $(AM_CFLAGS)
+libvirt_driver_vbox_la_LIBADD = $(DLOPEN_LIBS)
 if WITH_DRIVER_MODULES
+libvirt_driver_vbox_la_LIBADD += ../gnulib/lib/libgnu.la
 libvirt_driver_vbox_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 endif
-libvirt_driver_vbox_la_LIBADD = $(DLOPEN_LIBS)
 libvirt_driver_vbox_la_SOURCES = $(VBOX_DRIVER_SOURCES)
 endif
 
@@ -627,6 +631,7 @@ libvirt_driver_xenapi_la_CFLAGS = $(LIBXENSERVER_CFLAGS) 
$(LIBCURL_CFLAGS) \
 libvirt_driver_xenapi_la_LDFLAGS = $(AM_LDFLAGS)
 libvirt_driver_xenapi_la_LIBADD = $(LIBXENSERVER_LIBS) $(LIBCURL_LIBS)
 if WITH_DRIVER_MODULES
+libvirt_driver_xenapi_la_LIBADD += ../gnulib/lib/libgnu.la
 libvirt_driver_xenapi_la_LDFLAGS += -module -avoid-version
 endif
 libvirt_driver_xenapi_la_SOURCES = $(XENAPI_DRIVER_SOURCES)
@@ -645,6 +650,7 @@ libvirt_driver_qemu_la_CFLAGS = $(NUMACTL_CFLAGS) \
 libvirt_driver_qemu_la_LDFLAGS = $(AM_LDFLAGS)
 libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS)
 if WITH_DRIVER_MODULES
+libvirt_driver_qemu_la_LIBADD += ../gnulib/lib/libgnu.la
 libvirt_driver_qemu_la_LDFLAGS += -module -avoid-version
 endif
 libvirt_driver_qemu_la_SOURCES = $(QEMU_DRIVER_SOURCES)
@@ -670,6 +676,7 @@ endif
 libvirt_driver_lxc_la_CFLAGS = \
-...@top_srcdir@/src/conf $(AM_CFLAGS)
 if WITH_DRIVER_MODULES

[libvirt] [PATCH 1/4] Basic framework for auditing integration

[Daniel P. Berrange]

Integrate with libaudit.so for auditing of important operations.
libvirtd gains a couple of config entries for auditing. By
default it will enable auditing, if its enabled on the host.
It can be configured to force exit if auditing is disabled
on the host. It will can also send audit messages via libvirt
internal logging API

Places requiring audit reporting can use the VIR_AUDIT
macro to report data. This is a no-op unless auditing is
enabled

* autobuild.sh, mingw32-libvirt.spec.in: Disable audit
  on mingw
* configure.ac: Add check for libaudit
* daemon/libvirtd.aug, daemon/libvirtd.conf,
  daemon/test_libvirtd.aug, daemon/libvirtd.c: Add config
  options to enable auditing
* include/libvirt/virterror.h, src/util/virterror.c: Add
  VIR_FROM_AUDIT source
* libvirt.spec.in: Enable audit
* src/util/virtaudit.h, src/util/virtaudit.c: Simple internal
  API for auditing messages
---
 autobuild.sh|1 +
 configure.ac|   51 +++
 daemon/libvirtd.aug |4 +
 daemon/libvirtd.c   |   21 ++-
 daemon/libvirtd.conf|   19 ++
 daemon/test_libvirtd.aug|6 ++
 include/libvirt/virterror.h |1 +
 libvirt.spec.in |   13 
 mingw32-libvirt.spec.in |1 +
 po/POTFILES.in  |1 +
 src/Makefile.am |9 ++-
 src/util/virtaudit.c|  144 +++
 src/util/virtaudit.h|   55 
 src/util/virterror.c|3 +
 14 files changed, 325 insertions(+), 4 deletions(-)
 create mode 100644 src/util/virtaudit.c
 create mode 100644 src/util/virtaudit.h

diff --git a/autobuild.sh b/autobuild.sh
index c527479..844ce53 100755
--- a/autobuild.sh
+++ b/autobuild.sh
@@ -85,6 +85,7 @@ if [ -x /usr/bin/i686-pc-mingw32-gcc ]; then
 --without-one \
 --without-phyp \
 --without-netcf \
+--without-audit \
 --without-libvirtd
 
   make
diff --git a/configure.ac b/configure.ac
index bd92b65..5bf31da 100644
--- a/configure.ac
+++ b/configure.ac
@@ -911,6 +911,52 @@ AM_CONDITIONAL([HAVE_AVAHI], [test x$with_avahi = 
xyes])
 AC_SUBST([AVAHI_CFLAGS])
 AC_SUBST([AVAHI_LIBS])
 
+dnl Audit library
+AC_ARG_WITH([audit],
+  AC_HELP_STRING([--with-audit], [use audit library @:@default=check@:@]),
+  [],
+  [with_audit=check])
+
+AUDIT_CFLAGS=
+AUDIT_LIBS=
+if test $with_audit != no ; then
+  old_cflags=$CFLAGS
+  old_libs=$LIBS
+  if test $with_audit != check  $with_audit != yes ; then
+AUDIT_CFLAGS=-I$with_audit/include
+AUDIT_LIBS=-L$with_audit/lib
+  fi
+  CFLAGS=$CFLAGS $AUDIT_CFLAGS
+  LIBS=$LIBS $AUDIT_LIBS
+  fail=0
+  AC_CHECK_HEADER([libaudit.h], [], [fail=1])
+  AC_CHECK_LIB([audit], [audit_is_enabled], [], [fail=1])
+
+  if test $fail = 1 ; then
+if test $with_audit = yes ; then
+  AC_MSG_ERROR([You must install the Audit library in order to compile and 
run libvirt])
+else
+  with_audit=no
+  AUDIT_CFLAGS=
+  AUDIT_LIBS=
+fi
+  else
+with_audit=yes
+  fi
+
+  if test $with_audit = yes ; then
+AUDIT_LIBS=$AUDIT_LIBS -laudit
+AC_DEFINE_UNQUOTED([HAVE_AUDIT], 1, [whether libaudit is available])
+  fi
+
+  CFLAGS=$old_cflags
+  LIBS=$old_libs
+fi
+AM_CONDITIONAL([HAVE_AUDIT], [test $with_audit = yes])
+AC_SUBST([AUDIT_CFLAGS])
+AC_SUBST([AUDIT_LIBS])
+
+
 dnl SELinux
 AC_ARG_WITH([selinux],
   AC_HELP_STRING([--with-selinux], [use SELinux to manage security 
@:@default=check@:@]),
@@ -2273,6 +2319,11 @@ fi
 else
 AC_MSG_NOTICE([  polkit: no])
 fi
+if test $with_audit = yes ; then
+AC_MSG_NOTICE([   audit: $AUDIT_CFLAGS $AUDIT_LIBS])
+else
+AC_MSG_NOTICE([   audit: no])
+fi
 if test $with_selinux = yes ; then
 AC_MSG_NOTICE([ selinux: $SELINUX_CFLAGS $SELINUX_LIBS])
 else
diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
index 7406d23..0e06142 100644
--- a/daemon/libvirtd.aug
+++ b/daemon/libvirtd.aug
@@ -61,6 +61,9 @@ module Libvirtd =
  | str_entry log_filters
  | str_entry log_outputs
 
+   let auditing_entry = int_entry audit_level
+  | bool_entry audit_logging
+
(* Each enty in the config is one of the following three ... *)
let entry = network_entry
  | sock_acl_entry
@@ -69,6 +72,7 @@ module Libvirtd =
  | authorization_entry
  | processing_entry
  | logging_entry
+ | auditing_entry
let comment = [ label #comment . del /#[ \t]*/ #  .  store /([^ 
\t\n][^\n]*)?/ . del /\n/ \n ]
let empty = [ label #empty . eol ]
 
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 1543481..88e85ec 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -64,6 +64,7 @@
 #include memory.h
 #include stream.h
 #include hooks.h
+#include virtaudit.h
 #ifdef HAVE_AVAHI
 # include mdns.h
 #endif
@@ -187,6 +188,9 @@ static int max_requests = 20;
 /* Total number of 'in-process' RPC calls allowed by a single client*/
 static int max_client_requests = 5;
 

[libvirt] [PATCH 3/4] Audit SELinux label assignment.

[Daniel P. Berrange]

From: Miloslav Trmač m...@redhat.com

A more natural auditing point would perhaps be
SELinuxSetSecurityProcessLabel, but this happens in the child after root
permissions are dropped, so the kernel would refuse the audit record.
---
 src/security/security_selinux.c |   22 ++
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index a9dd836..0995d67 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -28,6 +28,8 @@
 #include pci.h
 #include hostusb.h
 #include storage_file.h
+#include uuid.h
+#include virtaudit.h
 
 #define VIR_FROM_THIS VIR_FROM_SECURITY
 
@@ -160,20 +162,22 @@ SELinuxGenSecurityLabel(virSecurityDriverPtr drv 
ATTRIBUTE_UNUSED,
 virDomainObjPtr vm)
 {
 int rc = -1;
-char mcs[1024];
+char mcs[1024], uuidstr[VIR_UUID_STRING_BUFLEN];
 char *scontext = NULL;
 int c1 = 0;
 int c2 = 0;
 
-if (vm-def-seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
-return 0;
+if (vm-def-seclabel.type == VIR_DOMAIN_SECLABEL_STATIC) {
+rc = 0;
+goto done;
+}
 
 if (vm-def-seclabel.label ||
 vm-def-seclabel.model ||
 vm-def-seclabel.imagelabel) {
 virSecurityReportError(VIR_ERR_INTERNAL_ERROR,
%s, _(security label already defined for 
VM));
-return rc;
+goto done;
 }
 
 do {
@@ -217,6 +221,16 @@ err:
 VIR_FREE(vm-def-seclabel.model);
 done:
 VIR_FREE(scontext);
+
+virUUIDFormat(vm-def-uuid, uuidstr);
+/* The derived socket context is not audited. */
+#define STR(X) ((X) != NULL ? (X) : ?)
+VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_ID, rc == 0,
+  name=%s uuid=%s process-context=%s image-context=%s,
+  vm-def-name, uuidstr, STR(vm-def-seclabel.label),
+  STR(vm-def-seclabel.imagelabel));
+#undef STR
+
 return rc;
 }
 
-- 
1.7.2.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 13/13] Remote protocol changes and implements virDomainSet/GetMemoryParameters

[Daniel P. Berrange]

On Tue, Oct 12, 2010 at 07:27:05PM +0200, Daniel Veillard wrote:
 On Fri, Oct 08, 2010 at 05:47:03PM +0530, Nikunj A. Dadhania wrote:
  From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
  
  v4:
  * prototype change: add unsigned int flags, regenerate files
  
  v3:
  * Squased all the remote driver changes to one single big patch and
auto-generated is around 40%
  * Implements domainSetMemoryParameters and domainGetMemoryParameters for 
  remote
driver
  daemon/remote.c
  src/remote/remote_driver.c
  
  * Auto generate the files using rpcgen and helper scripts in daemon/ 
  directory
  src/remote/remote_protocol.x
  daemon/remote_dispatch_args.h
  daemon/remote_dispatch_prototypes.h
  daemon/remote_dispatch_ret.h
  daemon/remote_dispatch_table.h
  src/remote/remote_protocol.c
  src/remote/remote_protocol.h
  
  Acked-by: Daniel P. Berrange berra...@redhat.com
  Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 [...]
   static int
  +remoteDispatchDomainSetMemoryParameters (struct qemud_server *server 
  ATTRIBUTE_UNUSED,
  + struct qemud_client *client 
  ATTRIBUTE_UNUSED,
  + virConnectPtr conn,
  + remote_message_header *hdr 
  ATTRIBUTE_UNUSED,
  + remote_error *rerr,
  + 
  remote_domain_set_memory_parameters_args *args,
  + void *ret ATTRIBUTE_UNUSED)
  +{
  +virDomainPtr dom;
  +int i, r, nparams;
  +virMemoryParameterPtr params;
  +unsigned int flags;
  +
  +nparams = args-params.params_len;
  +nparams = args-flags;
 
   obvious bug: flags = args-flags;
 
 That reminds me that I didn't see flags being checked against 0 on the
 QEmu and LXC drivers, we should check them !
 
  +if (nparams  REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) {
  +remoteDispatchFormatError (rerr, %s, _(nparams too large));
  +return -1;
 
   I did a lot of reformating and code cleanups in the non-generated
   files, to try to keep the source readable on a 80 columns editor.
 
  diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
  index 8af469c..f5dcb5c 100644
  --- a/src/remote/remote_protocol.x
  +++ b/src/remote/remote_protocol.x
  @@ -128,6 +128,9 @@ const REMOTE_NWFILTER_NAME_LIST_MAX = 1024;
   /* Upper limit on list of scheduler parameters. */
   const REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX = 16;
   
  +/* Upper limit on list of memory parameters. */
  +const REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX = 16;
  +
 
   Hopefully we won't exhaust that crucial limit

That limit isn't ABI critical. It is simply to prevent DOS when
de-marshalling data, so we can raise it at will in the future.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London-o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org-o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/8] virsh: allow zero length arguments

[Eric Blake]

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:

the following command is allowed at shell, we also make it allowed at virsh 
shel.


s/shel/shell/


# somecmd 

Signed-off-by: Lai Jiangshanla...@cn.fujitsu.com
---


ACK.  But I'm wondering what sort of subtle surprises we might now get 
in any client that is not prepared to handle an empty argument.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/4] Fix symbol exports remove duplicated libvirt_util.la linkage

[Eric Blake]

On 10/12/2010 11:32 AM, Daniel P. Berrange wrote:

@@ -708,6 +735,18 @@ virFileFindMountPoint;
  virFileWaitForDevices;
  virFileMatchesNameSuffix;
  virArgvToString;
+virStrcpy;
+virStrncpy;
+virBuildPathInternal;
+virFileStripSuffix;
+virFileOperation;
+virFork;
+virRandom;
+virRandomInitialize;
+virDirCreate;
+virIndexToDiskName;
+virHexToBin;


This is a duplicate with the patch I committed earlier today (I only 
noticed one function, but you obviously linked shard modules and found a 
lot more).  Should we be running this through sort | uniq -c and 
detecting any duplicate lines as part of 'make syntax-check'?


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/8] virsh: better support double quote

[Eric Blake]

On 10/12/2010 10:50 AM, Eric Blake wrote:

Hmm, one thing you _don't_ recognize is:

--option

as an option. In the shell, quotes are stripped before arguments are
recognized as a particular token. I think that's pretty easy to support
- delay VSH_TK_OPTION checking until after we've stripped quotes, but
I'll show it as a separate patch.


Nevermind.  Patch 3 drops VSH_TK_OPTION altogether, at which point 
you've already delayed -- detection and nicely solved this problem 
without me needing any followup for that issue.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/8] virsh: better handling the boolean option

[Eric Blake]

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:

in old code the following commands are equivalent:
  virsh # dumpxml --update-cpu=vm1
  virsh # dumpxml --update-cpu vm1
because the old code split the option argument into 2 parts:
--update-cpu=vm1 is split into update-cpu and vm1,
and update-cpu is a boolean option, so the parser takes vm1 as another
argument, very strange.

after this patch applied, the first one will become illegal.

To achieve this, we don't parse/check options when parsing command sting,
but check options when parsing a command argument. And the argument is
not split when parsing command sting.

Signed-off-by: Lai Jiangshanla...@cn.fujitsu.com
+++ b/tools/virsh.c
@@ -10164,14 +10164,12 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
   */
  #define VSH_TK_ERROR-1
  #define VSH_TK_NONE0
-#define VSH_TK_OPTION1
-#define VSH_TK_DATA2
-#define VSH_TK_END3
+#define VSH_TK_DATA1
+#define VSH_TK_END2


Maybe it would be nice to make these an enum rather than #defines, but 
that would be a separate cleanup patch, no impact to today's review.



@@ -10296,18 +10278,28 @@ vshCommandParse(vshControl *ctl, char *cmdstr)
  goto syntaxError;   /* ... or ignore this command only? */
  }
  VIR_FREE(tkdata);
-} else if (tk == VSH_TK_OPTION) {
-if (!(opt = vshCmddefGetOption(cmd, tkdata))) {
+} else if (*tkdata == '-'  *(tkdata + 1) == '-'  *(tkdata + 2)
+  c_isalnum(*(tkdata + 2))) {
+char *optstr = strchr(tkdata + 2, '=');
+if (optstr) {
+*optstr = '\0'; /* conver the '=' to '\0' */


s/conver/convert/

ACK with that nit fixed.

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] KVM Windows guest: add hard drive

[Mihamina Rakotomandimby]

Manao ahoana, Hello, Bonjour,

I installed a Windows (XP and 2003) guest using one partition as hard
drive.
I used:

  sudo virt-install --connect qemu:///system \
   --name win2003-01 \
   --ram 1024 \
   --keymap=fr \
   --disk path=/dev/lvm0/win2003-01 \
   --cdrom=/home/mihamina/Windows_2003_Server.iso \
   --os-type windows \
   --os-variant=win2k3 \
   --noapic \
   --noacpi \
   --network=bridge:br0 \
   --accelerate \
   --vnc \
   --force

And this works fine. Really fine.


The disk section in the definition:

disk type='block' device='disk'
  source dev='/dev/lvm0/win2003-01'/
  target dev='hda' bus='ide'/
/disk

After installation, people asked me to add another hard drive to this
installation, and then, I added another disk in the definition:

disk type='block' device='disk'
  source dev='/dev/lvm0/win2003-01'/
  target dev='hda' bus='ide'/
/disk
disk type='block' device='disk'
  source dev='/dev/lvm0/win2003-01-a'/
  target dev='hdb' bus='ide'/
/disk

I destroyed + undefined + defined + started the VM and the definition
is OK (dumpxml), but the Windows guest doesnt see any new hard drive. 

I already tried with replacing ide with virtio, no new hard drive
yet.

This is my configuration:
miham...@kvm-lxc-02:~$ dpkg -l | grep virt
ii  kvm 72+dfsg-5~lenny5   Full 
virtualization on x86 hardware
ii  libvirt-bin 0.4.6-10   the programs 
for the libvirt library
ii  libvirt00.4.6-10   library for 
interfacing with different virtualization systems
ii  python-libvirt  0.4.6-10   libvirt 
Python bindings
ii  virt-manager0.6.0-6desktop 
application for managing virtual machines
ii  virt-viewer 0.0.3-2Displaying 
the graphical console of a virtual machine
ii  virtinst0.400.0-7  Programs to 
create and clone virtual machines

How to make, with this, new hard drive recognized by the Windows guest? 

Misaotra, Thanks, Merci.


-- 

   Architecte Informatique chez Blueline/Gulfsat:
Administration Systeme, Recherche  Developpement
+261 34 56 000 19

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] KVM Windows guest: add hard drive

[Andrew Cathrow]

- Original Message -
 From: Mihamina Rakotomandimby miham...@gulfsat.mg
 To: virt-tools-l...@redhat.com, Libvirt Developers Mailing List 
 libvir-list@redhat.com
 Sent: Tuesday, October 12, 2010 2:17:49 PM
 Subject: [libvirt] KVM Windows guest: add hard drive
 Manao ahoana, Hello, Bonjour,
 
 I installed a Windows (XP and 2003) guest using one partition as hard
 drive.
 I used:
 
 sudo virt-install --connect qemu:///system \
 --name win2003-01 \
 --ram 1024 \
 --keymap=fr \
 --disk path=/dev/lvm0/win2003-01 \
 --cdrom=/home/mihamina/Windows_2003_Server.iso \
 --os-type windows \
 --os-variant=win2k3 \
 --noapic \
 --noacpi \
 --network=bridge:br0 \
 --accelerate \
 --vnc \
 --force
 
 And this works fine. Really fine.
 
 
 The disk section in the definition:
 
 disk type='block' device='disk'
 source dev='/dev/lvm0/win2003-01'/
 target dev='hda' bus='ide'/
 /disk
 
 After installation, people asked me to add another hard drive to this
 installation, and then, I added another disk in the definition:
 
 disk type='block' device='disk'
 source dev='/dev/lvm0/win2003-01'/
 target dev='hda' bus='ide'/
 /disk
 disk type='block' device='disk'
 source dev='/dev/lvm0/win2003-01-a'/
 target dev='hdb' bus='ide'/
 /disk
 
 I destroyed + undefined + defined + started the VM and the definition
 is OK (dumpxml), but the Windows guest doesnt see any new hard drive.
 
 I already tried with replacing ide with virtio, no new hard drive
 yet.
 
 This is my configuration:
 miham...@kvm-lxc-02:~$ dpkg -l | grep virt
 ii kvm 72+dfsg-5~lenny5 Full virtualization on x86 hardware
 ii libvirt-bin 0.4.6-10 the programs for the libvirt library
 ii libvirt0 0.4.6-10 library for interfacing with different
 virtualization systems
 ii python-libvirt 0.4.6-10 libvirt Python bindings
 ii virt-manager 0.6.0-6 desktop application for managing virtual
 machines
 ii virt-viewer 0.0.3-2 Displaying the graphical console of a virtual
 machine
 ii virtinst 0.400.0-7 Programs to create and clone virtual machines
 
 How to make, with this, new hard drive recognized by the Windows
 guest?

Are you looking for a new driver to appear in 'my computer', because it 
wouldn't appear there until you format it.
Does it show up in device manager?



 
 Misaotra, Thanks, Merci.
 
 
 --
 
 Architecte Informatique chez Blueline/Gulfsat:
 Administration Systeme, Recherche  Developpement
 +261 34 56 000 19
 
 --
 libvir-list mailing list
 libvir-list@redhat.com
 https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 0/8] virsh: rework command parsing

[Laine Stump]

 On 10/12/2010 12:26 PM, Eric Blake wrote:


Now that I've done a bit more research; here's the results, to make it 
easier for others to do:


git config sendemail.chainreplyto false
git config sendemail.thread false
git config format.thread shallow

Then:

git send-email -8 --cover-letter

will automatically send 8 messages all in reply to the cover letter, 
creating a single thread.




Odd that it should require any configuring - mine has just always worked 
that way when I do, eg:


   git send-email -8 --compose

(I hadn't seen --cover-letter before. The man page for git-send-email 
only lists --compose, and I see the man page for git-format-patch only 
lists --cover-letter, but git send-email accepts both; I assume they're 
synonyms)


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Fix several minor problems introduced by the memtune series

[Matthias Bolte]

Add proper documentation to the new VIR_DOMAIN_MEMORY_* macros in
libvirt.h.in to placate apibuild.py.

Mark args as unused in for libvirt_virDomain{Get,Set}MemoryParameters
in the Python bindings and add both to the libvirtMethods array.

Update remote_protocol-structs to placate make syntax-check.

Undo unintended modifications in vboxDomainGetInfo.

Update the function table of the VirtualBox and XenAPI drivers.
---
 include/libvirt/libvirt.h.in |   28 
 python/libvirt-override.c|6 --
 src/remote_protocol-structs  |   35 +++
 src/vbox/vbox_tmpl.c |6 --
 src/xenapi/xenapi_driver.c   |2 ++
 5 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index e244eac..ca8e6fa 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -695,9 +695,37 @@ typedef enum {
  */
 
 #define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80
+
+/**
+ * VIR_DOMAIN_MEMORY_HARD_LIMIT:
+ *
+ * Macro for the well-known tunable hard_limit.
+ */
+
 #define VIR_DOMAIN_MEMORY_HARD_LIMIT hard_limit
+
+/**
+ * VIR_DOMAIN_MEMORY_SOFT_LIMIT:
+ *
+ * Macro for the well-known tunable soft_limit.
+ */
+
 #define VIR_DOMAIN_MEMORY_SOFT_LIMIT soft_limit
+
+/**
+ * VIR_DOMAIN_MEMORY_MIN_GUARANTEE:
+ *
+ * Macro for the well-known tunable min_guarantee.
+ */
+
 #define VIR_DOMAIN_MEMORY_MIN_GUARANTEE min_guarantee
+
+/**
+ * VIR_DOMAIN_SWAP_HARD_LIMIT:
+ *
+ * Macro for the well-known tunable swap_hard_limit.
+ */
+
 #define VIR_DOMAIN_SWAP_HARD_LIMIT swap_hard_limit
 
 /**
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index c43ab15..4a03d72 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -374,14 +374,14 @@ libvirt_virDomainSetSchedulerParameters(PyObject *self 
ATTRIBUTE_UNUSED,
 /* FIXME: This is a place holder for the implementation. */
 static PyObject *
 libvirt_virDomainSetMemoryParameters(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args ATTRIBUTE_UNUSED) {
 return VIR_PY_INT_FAIL;
 }
 
 /* FIXME: This is a place holder for the implementation. */
 static PyObject *
 libvirt_virDomainGetMemoryParameters(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args ATTRIBUTE_UNUSED) {
 return VIR_PY_INT_FAIL;
 }
 
@@ -3532,6 +3532,8 @@ static PyMethodDef libvirtMethods[] = {
 {(char *) virDomainGetSchedulerType, libvirt_virDomainGetSchedulerType, 
METH_VARARGS, NULL},
 {(char *) virDomainGetSchedulerParameters, 
libvirt_virDomainGetSchedulerParameters, METH_VARARGS, NULL},
 {(char *) virDomainSetSchedulerParameters, 
libvirt_virDomainSetSchedulerParameters, METH_VARARGS, NULL},
+{(char *) virDomainSetMemoryParameters, 
libvirt_virDomainSetMemoryParameters, METH_VARARGS, NULL},
+{(char *) virDomainGetMemoryParameters, 
libvirt_virDomainGetMemoryParameters, METH_VARARGS, NULL},
 {(char *) virDomainGetVcpus, libvirt_virDomainGetVcpus, METH_VARARGS, 
NULL},
 {(char *) virDomainPinVcpu, libvirt_virDomainPinVcpu, METH_VARARGS, 
NULL},
 {(char *) virConnectListStoragePools, 
libvirt_virConnectListStoragePools, METH_VARARGS, NULL},
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index a5fc6aa..838423e 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -70,6 +70,21 @@ struct remote_sched_param {
remote_nonnull_string  field;
remote_sched_param_value   value;
 };
+struct remote_memory_param_value {
+   inttype;
+   union {
+   inti;
+   u_int  ui;
+   int64_tl;
+   uint64_t   ul;
+   double d;
+   intb;
+   } remote_memory_param_value_u;
+};
+struct remote_memory_param {
+   remote_nonnull_string  field;
+   remote_memory_param_value  value;
+};
 struct remote_open_args {
remote_string  name;
intflags;
@@ -151,6 +166,26 @@ struct remote_domain_set_scheduler_parameters_args {
remote_sched_param * params_val;
} params;
 };
+struct remote_domain_set_memory_parameters_args {
+   remote_nonnull_domain  dom;
+   struct {
+   u_int  params_len;
+   remote_memory_param * params_val;
+   } params;
+   u_int  flags;
+};
+struct remote_domain_get_memory_parameters_args {
+   remote_nonnull_domain  dom;
+   intnparams;
+   u_int  flags;
+};
+struct remote_domain_get_memory_parameters_ret {
+   struct {
+   u_int  params_len;
+   

Re: [libvirt] [PATCH 4/8] virsh: add vrshCommandParser abstraction

[Eric Blake]

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:


add vrshCommandParser and make vshCommandParse() accepts different


s/vrsh/vsh/; s/accepts/accept/


parsers.

the current code for parse command string is integrated as
vshCommandStringParse().

Signed-off-by: Lai Jiangshanla...@cn.fujitsu.com
---
  virsh.c |   91 
+++-
  1 file changed, 45 insertions(+), 46 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index f97ee42..27321a5 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10158,23 +10158,29 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
  return ret;
  }

+#define VSH_TK_ERROR   -1
+#define VSH_TK_ARG 0
+#define VSH_TK_SUBCMD_END  1
+#define VSH_TK_END 2


Hmm, in addition to floating this earlier, you also lost _NONE and added 
_SUBCMD_END.  The enum I suggested in patch 3 is sounding more 
appealing, so I'll squash it in now.



+
+typedef struct __vshCommandParser {
+int (*getNextArg)(vshControl *, struct __vshCommandParser *, char **);
+char *pos;
+} vshCommandParser;
+
+static int vshCommandParse(vshControl *ctl, vshCommandParser *parser);


I'm a fan of avoiding forward static declarations if the file can be 
rearranged topologically.



+
  /* ---
   * Command string parsing
   * ---
   */


This logically belongs before enums related to command parsing.


-#define VSH_TK_ERROR-1
-#define VSH_TK_NONE0
-#define VSH_TK_DATA1
-#define VSH_TK_END2

  static int
-vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
+vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res)
  {
  bool double_quote = false;
  int sz = 0;
-char *p = str, *q;
-
-*end = NULL;
+char *p = parser-pos, *q;


Rebasing this on top of my edits to patch 1 is getting interesting.



  while (p  *p  (*p == ' ' || *p == '\t'))


You no longer need to check if p is NULL,


+static int vshCommandStringParse(vshControl *ctl, char *cmdstr)
+{
+vshCommandParser parser;
+
+if (cmdstr == NULL || *cmdstr == '\0')
+return FALSE;
+
+parser.pos = cmdstr;


...since this guarantees it is non-NULL.


+for (;;) {


$ git grep 'for.*;;' | wc -l
14
$ git grep 'while.*true' | wc -l
6
$ git grep 'while.*1' | wc -l
70

Guess which one I'm changing this to, for consistency :)

ACK, with those nits fixed.  Here's what I squashed in:

diff --git i/tools/virsh.c w/tools/virsh.c
index 56985a4..d9f72f3 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10174,38 +10174,40 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
 return ret;
 }

-#define VSH_TK_ERROR   -1
-#define VSH_TK_ARG 0
-#define VSH_TK_SUBCMD_END  1
-#define VSH_TK_END 2
+/* ---
+ * Command string parsing
+ * ---
+ */
+
+typedef enum {
+VSH_TK_ERROR, /* Failed to parse a token */
+VSH_TK_ARG, /* Arbitrary argument, might be option or empty */
+VSH_TK_SUBCMD_END, /* Separation between commands */
+VSH_TK_END /* No more commands */
+} vshCommandToken;

 typedef struct __vshCommandParser {
-int (*getNextArg)(vshControl *, struct __vshCommandParser *, char **);
+vshCommandToken (*getNextArg)(vshControl *, struct 
__vshCommandParser *,

+  char **);
 char *pos;
 } vshCommandParser;

 static int vshCommandParse(vshControl *ctl, vshCommandParser *parser);

-/* ---
- * Command string parsing
- * ---
- */
-
-static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+static vshCommandToken ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
 vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char 
**res)

 {
 bool double_quote = false;
 int sz = 0;
 char *p = parser-pos;
-char *q = vshStrdup(ctl, str);
+char *q = vshStrdup(ctl, p);

-*end = NULL;
 *res = q;

-while (p  *p  (*p == ' ' || *p == '\t'))
+while (*p  (*p == ' ' || *p == '\t'))
 p++;

-if (p == NULL || *p == '\0')
+if (*p == '\0')
 return VSH_TK_END;
 if (*p == ';') {
 parser-pos = ++p; /* = \0 or begin of next command */
@@ -10261,15 +10262,15 @@ vshCommandParse(vshControl *ctl, 
vshCommandParser *parser)

 ctl-cmd = NULL;
 }

-for (;;) {
+while (1) {
 vshCmdOpt *last = NULL;
 const vshCmdDef *cmd = NULL;
-int tk;
+vshCommandToken tk;
 int data_ct = 0;

 first = NULL;

-for (;;) {
+while (1) {
 const vshCmdOptDef *opt = NULL;

 tkdata = NULL;

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix several minor problems introduced by the memtune series

[Matthias Bolte]

2010/10/12 Eric Blake ebl...@redhat.com:
 On 10/12/2010 01:29 PM, Matthias Bolte wrote:

 Add proper documentation to the new VIR_DOMAIN_MEMORY_* macros in
 libvirt.h.in to placate apibuild.py.

 Mark args as unused in for libvirt_virDomain{Get,Set}MemoryParameters
 in the Python bindings and add both to the libvirtMethods array.

 Update remote_protocol-structs to placate make syntax-check.

 Undo unintended modifications in vboxDomainGetInfo.

 Update the function table of the VirtualBox and XenAPI drivers.
 ---
  include/libvirt/libvirt.h.in |   28 
  python/libvirt-override.c    |    6 --
  src/remote_protocol-structs  |   35 +++
  src/vbox/vbox_tmpl.c         |    6 --
  src/xenapi/xenapi_driver.c   |    2 ++
  5 files changed, 73 insertions(+), 4 deletions(-)

 ACK.


Thanks, pushed.

Matthias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/8] virsh: rework command parsing

[Eric Blake]

On 10/12/2010 01:14 AM, Lai Jiangshan wrote:

Old virsh command parsing mashes all the args back into a string and
miss the quotes, this patches fix it. It is also needed for introducing
qemu-monitor-command which is very useful.

This patches uses the new vrshCommandParser abstraction and adds


s/vrsh/vsh/


+++ b/tools/virsh.c
@@ -10165,12 +10165,47 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)

  typedef struct __vshCommandParser {
  int (*getNextArg)(vshControl *, struct __vshCommandParser *, char **);
+/* vshCommandStringGetArg() */
  char *pos;
+/* vshCommandArgvGetArg() */
+char **arg_pos;
+char **arg_end;


If I was worried about memory, I'd consider making this a discriminated 
union; but we don't instantiate too many vshCommandParser objects to be 
worth worrying about the extra word of storage.



+/* ---
   * Command string parsing
   * ---
   */
@@ -10939,7 +10974,8 @@ static void
  vshUsage(void)
  {
  const vshCmdDef *cmd;
-fprintf(stdout, _(\n%s [options] [commands]\n\n
+fprintf(stdout, _(\n%s [options]... [command_string]
+  \n%s [options]...command  [args...]\n\n


Hmm; we need a corresponding patch to virsh.pod.

ACK. Here's what I'm squashing as part of rebasing (yes, I documented 
features that aren't in until later patches; oh well).  And I'm posting 
a followup patch that documents virsh's options, like -c.


diff --git i/tools/virsh.c w/tools/virsh.c
index 901b953..688705d 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10203,7 +10203,7 @@ static int vshCommandParse(vshControl *ctl, 
vshCommandParser *parser);

  * ---
  */

-static int
+static vshCommandToken ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
 vshCommandArgvGetArg(vshControl *ctl, vshCommandParser *parser, char 
**res)

 {
 if (parser-arg_pos == parser-arg_end) {
diff --git i/tools/virsh.pod w/tools/virsh.pod
index e0471b1..209aa54 100644
--- i/tools/virsh.pod
+++ w/tools/virsh.pod
@@ -4,7 +4,9 @@ virsh - management user interface

 =head1 SYNOPSIS

-virsh subcommand [args]
+Bvirsh [IOPTION]... [ICOMMAND_STRING]
+
+Bvirsh [IOPTION]... ICOMMAND [IARG]...

 =head1 DESCRIPTION

@@ -22,20 +24,25 @@ KVM, LXC, OpenVZ, VirtualBox, OpenNebula, and VMware 
ESX.


 The basic structure of most virsh usage is:

-  virsh command domain-id [OPTIONS]
+  virsh command domain-id [ARG]...

 Where Icommand is one of the commands listed below, Idomain-id
 is the numeric domain id, or the domain name (which will be internally
-translated to domain id), and IOPTIONS are command specific
+translated to domain id), and IARGS are command specific
 options.  There are a few exceptions to this rule in the cases where
 the command in question acts on all domains, the entire machine,
 or directly on the xen hypervisor.  Those exceptions will be clear for
 each of those commands.

-The Bvirsh program can be used either to run one command at a time
-by giving the command as an argument on the command line, or as a shell
-if no command is given in the command line, it will then start a minimal
-interpreter waiting for your commands and the Bquit command will then 
exit

+The Bvirsh program can be used either to run one ICOMMAND by giving the
+command and its arguments on the shell command line, or a ICOMMAND_STRING
+which is a single shell argument consisting of multiple ICOMMAND actions
+and their arguments joined with whitespace, and separated by semicolons
+between commands.  Within ICOMMAND_STRING, virsh understands the
+same single, double, and backslash escapes as the shell, although you must
+add another layer of shell escaping in creating the single shell argument.
+If no command is given in the command line, Bvirsh will then start a 
minimal
+interpreter waiting for your commands, and the Bquit command will 
then exit

 the program.

 =head1 NOTES

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virsh: document options in man page

[Eric Blake]

* tools/virsh.pod: Document top-level options.
---

 And I'm posting a followup patch that documents virsh's options, like -c.

Can't believe we didn't have this in the man page!

 tools/virsh.pod |   45 -
 1 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 209aa54..f65f6d4 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -24,7 +24,7 @@ KVM, LXC, OpenVZ, VirtualBox, OpenNebula, and VMware ESX.

 The basic structure of most virsh usage is:

-  virsh command domain-id [ARG]...
+  virsh [OPTION]... command domain-id [ARG]...

 Where Icommand is one of the commands listed below, Idomain-id
 is the numeric domain id, or the domain name (which will be internally
@@ -45,6 +45,49 @@ If no command is given in the command line, Bvirsh will 
then start a minimal
 interpreter waiting for your commands, and the Bquit command will then exit
 the program.

+The Bvirsh program understands the following IOPTIONS.
+
+=over 4
+
+=item B-h, B--help
+
+Ignore all other arguments, and behave as if the Bhelp command were
+given instead.
+
+=item B-v, B--version
+
+Ignore all other arguments, and behave as if the Bversion command were
+given instead.
+
+=item B-c, B--connect IURI
+
+Connect to the specified IURI, as if by the Bconnect command,
+instead of the default connection.
+
+=item B-d, B--debug ILEVEL
+
+Enable debug messages at integer ILEVEL and above.  ILEVEL can
+range from 0 (default) to 5.
+
+=item B-l, B--log IFILE
+
+Output logging details to IFILE.
+
+=item B-q, B--quiet
+
+Avoid extra informational messages.
+
+=item B-r, B--readonly
+
+Make the initial connection read-only, as if by the I--readonly
+option of the Bconnect command.
+
+=item B-t, B--timing
+
+Output elapsed time information for each command.
+
+=back
+
 =head1 NOTES

 Most Bvirsh operations rely upon the libvirt library being able to
-- 
1.7.2.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 6/8] virsh: add escaper \ for command string parsing

[Eric Blake]

On 10/12/2010 01:14 AM, Lai Jiangshan wrote:

add escaper \ for command string parsing, example:

virsh # cd /path/which/have/a/double\quote

Signed-off-by: Lai Jiangshanla...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index 9fd0602..b96071d 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10235,7 +10235,11 @@ copy:
  if (!double_quote  (*p == ' ' || *p == '\t' || *p == ';'))
  break;

-   if (*p == '') {
+if (*p == '\\') { /* escape */
+p++;
+if (*p == '\0')
+break;


Hmm - dangling \ should be an error, rather than silently discarded.


+} else if (*p == '') { /* double quote */
  double_quote = !double_quote;
  p++;
  continue;



ACK with that fixed; here's what I'm squashing in.

diff --git i/tools/virsh.c w/tools/virsh.c
index d49d18a..16d141c 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10261,8 +10261,10 @@ vshCommandStringGetArg(vshControl *ctl, 
vshCommandParser *parser, char **res)


 if (*p == '\\') { /* escape */
 p++;
-if (*p == '\0')
-break;
+if (*p == '\0') {
+vshError(ctl, %s, _(dangling \\));
+return VSH_TK_ERROR;
+}
 } else if (*p == '') { /* double quote */
 double_quote = !double_quote;
 p++;

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 7/8] virsh: support single quote

[Eric Blake]

On 10/12/2010 01:14 AM, Lai Jiangshan wrote:

Some use may type command like this at the virsh shell:
virsh # somecmd 'some arg'

because some users often use single quote in linux shell.

Signed-off-by: Lai Jiangshanla...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index b96071d..a5b438b 100644


Tests would be nice.  I'll see about adding some in another patch, 
probably by adding a 'virsh echo ...' command that echoes its arguments 
for reuse.



+if (!double_quote  !single_quote
+  (*p == ' ' || *p == '\t' || *p == ';'))


Convention on this project is to line break after operators rather than 
before.  It's not a hard-fast rule, but as long as I'm on a roll of 
tweaking every one of your patches... :)  [And pardon Thunderbird's 
stupid bug that mangles the spacing before any word beginning with , , 
or  in the quoted portions of my message.]


ACK with this squashed in:

diff --git i/tools/virsh.c w/tools/virsh.c
index c38f91d..e21bbf2 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10257,8 +10257,8 @@ vshCommandStringGetArg(vshControl *ctl, 
vshCommandParser *parser, char **res)


 while (*p) {
 /* end of token is blank space or ';' */
-if (!double_quote  !single_quote
- (*p == ' ' || *p == '\t' || *p == ';'))
+if (!double_quote  !single_quote 
+(*p == ' ' || *p == '\t' || *p == ';'))
 break;

 if (!double_quote  *p == '\'') { /* single quote */

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 8/8] virsh: add -- support

[Eric Blake]

On 10/12/2010 01:14 AM, Lai Jiangshan wrote:


-- means no option at the following arguments.

Signed-off-by: Lai Jiangshanla...@cn.fujitsu.com
---
diff --git a/tools/virsh.c b/tools/virsh.c
index a5b438b..d01091f 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10305,6 +10305,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
  vshCmdOpt *last = NULL;
  const vshCmdDef *cmd = NULL;
  int tk;
+bool data_only = false;
  int data_ct = 0;

  first = NULL;
@@ -10327,6 +10328,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
  goto syntaxError;   /* ... or ignore this command only? */
  }
  VIR_FREE(tkdata);
+} else if (data_only) {
+goto get_data;
  } else if (*tkdata == '-'  *(tkdata + 1) == '-'  *(tkdata + 2)
   c_isalnum(*(tkdata + 2))) {
  char *optstr = strchr(tkdata + 2, '=');
@@ -10368,7 +10371,12 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
  goto syntaxError;
  }
  }
+} else if (*tkdata == '-'  *(tkdata + 1) == '-'
+  *(tkdata + 2) == '\0') {


Another case of line break convention.  Also, I prefer tkdata[1] over 
*(tkdata + 1).


Hmm, that means there's now two levels of -- parsing - one to mark the 
end of top-level virsh commands, and one for each command.  That is:


virsh -- help -- help

should be the same as

virsh help help

Which also means that virsh should probably be avoiding argv 
rearrangement in getopt_long().  That is, given my theoretical echo command,


virsh echo --help

should echo --help, rather than running 'virsh --help'.  Or, more to 
the point,


virsh dumpxml --update-cpu vm

correctly avoids promoting --update-cpu to a top-level argument of virsh.

Oh my - I just looked in the code, and virsh is re-doing option parsing 
by itself, instead of just telling getopt_long() to stop on the first 
non-option; but getting it wrong by not checking for abbreviations. 
Another patch or two coming up...


ACK with the nit fixed.  Here's what I'm squashing.

diff --git i/tools/virsh.c w/tools/virsh.c
index 79d7756..8c4a7bc 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10347,8 +10347,8 @@ vshCommandParse(vshControl *ctl, 
vshCommandParser *parser)

 VIR_FREE(tkdata);
 } else if (data_only) {
 goto get_data;
-} else if (*tkdata == '-'  *(tkdata + 1) == '-'  
*(tkdata + 2)

-c_isalnum(*(tkdata + 2))) {
+} else if (tkdata[0] == '-'  tkdata[1] == '-' 
+   c_isalnum(tkdata[2])) {
 char *optstr = strchr(tkdata + 2, '=');
 if (optstr) {
 *optstr = '\0'; /* convert the '=' to '\0' */
@@ -10388,8 +10388,8 @@ vshCommandParse(vshControl *ctl, 
vshCommandParser *parser)

 goto syntaxError;
 }
 }
-} else if (*tkdata == '-'  *(tkdata + 1) == '-'
-*(tkdata + 2) == '\0') {
+} else if (tkdata[0] == '-'  tkdata[1] == '-' 
+   tkdata[2] == '\0') {
 data_only = true;
 continue;
 } else {


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 04/13] XML parsing for memory tunables

[Daniel Veillard]

On Tue, Oct 12, 2010 at 09:33:03PM +0200, Matthias Bolte wrote:
 2010/10/12 Daniel Veillard veill...@redhat.com:
  On Fri, Oct 08, 2010 at 05:45:23PM +0530, Nikunj A. Dadhania wrote:
  From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
  Adding parsing code for memory tunables in the domain xml file
 
  v4:
  * Add memtune in tests/qemuxml2xmltest.c
  * Fix: insert memtune element only when any of them is set
 
  v2:
  + Fix typo min_guarantee
 
   The patch is fine except the usual space and tabs mixups and the fact
  that a number of drivers still needed to be converted to the change
  of the definition structure. grep -- -memory src/*/* isn't that
  hard and would have shown that even the driver for your own IBM Phyp
  hardware failed to compile after your patch !!
 
   anyway once cleaned up the patch makes sensei, ACK, but please use
  make syntax-check and do not configure out drivers when you are
  developping patches,
 
   thanks,
 
  Daniel
 
 
 This patch should have added documentation about the new XML elements
 to docs/formatdomain.html.in.

  right! virsh man page need to be completed too,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virsh: simplify top-level option parsing

[Eric Blake]

This makes 'virsh --conn test:///default help help' work right;
previously, the abbreviation confused our hand-rolled option parsing.

* tools/virsh.c (vshParseArgv): Use getopt_long feature, rather
than (incorrectly) reparsing options ourselves.
---

 Oh my - I just looked in the code, and virsh is re-doing option
 parsing by itself, instead of just telling getopt_long() to stop on
 the first non-option; but getting it wrong by not checking for
 abbreviations. Another patch or two coming up...

I love patches that nuke more code than they add, all while fixing
bugs at the same time!

 tools/virsh.c |   68 +---
 1 files changed, 16 insertions(+), 52 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 8c4a7bc..19a6087 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -10991,9 +10991,8 @@ vshUsage(void)
 static int
 vshParseArgv(vshControl *ctl, int argc, char **argv)
 {
-char *last = NULL;
-int i, end = 0, help = 0;
-int arg, idx = 0;
+bool help = false;
+int arg;
 struct option opt[] = {
 {debug, 1, 0, 'd'},
 {help, 0, 0, 'h'},
@@ -11006,46 +11005,10 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
 {0, 0, 0, 0}
 };

-
-if (argc  2)
-return TRUE;
-
-/* look for begin of the command, for example:
- *   ./virsh --debug 5 -q command --cmdoption
- *  --- ^ ---
- *getopt() stuff | command suff
- */
-for (i = 1; i  argc; i++) {
-if (*argv[i] != '-') {
-int valid = FALSE;
-
-/* non --option argv, is it command? */
-if (last) {
-struct option *o;
-int sz = strlen(last);
-
-for (o = opt; o-name; o++) {
-if (o-has_arg == 1){
-if (sz == 2  *(last + 1) == o-val)
-/* valid virsh short option */
-valid = TRUE;
-else if (sz  2  STREQ(o-name, last + 2))
-/* valid virsh long option */
-valid = TRUE;
-}
-}
-}
-if (!valid) {
-end = i;
-break;
-}
-}
-last = argv[i];
-}
-end = end ? end : argc;
-
-/* standard (non-command) options */
-while ((arg = getopt_long(end, argv, d:hqtc:vrl:, opt, idx)) != -1) {
+/* Standard (non-command) options. The leading + ensures that no
+ * argument reordering takes place, so that command options are
+ * not confused with top-level virsh options. */
+while ((arg = getopt_long(argc, argv, +d:hqtc:vrl:, opt, NULL)) != -1) {
 switch (arg) {
 case 'd':
 if (virStrToLong_i(optarg, NULL, 10, ctl-debug)  0) {
@@ -11054,7 +11017,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
 }
 break;
 case 'h':
-help = 1;
+help = true;
 break;
 case 'q':
 ctl-quiet = TRUE;
@@ -11066,7 +11029,8 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
 ctl-name = vshStrdup(ctl, optarg);
 break;
 case 'v':
-fprintf(stdout, %s\n, VERSION);
+/* FIXME - list a copyright blurb, as in GNU programs?  */
+puts(VERSION);
 exit(EXIT_SUCCESS);
 case 'r':
 ctl-readonly = TRUE;
@@ -11081,8 +11045,8 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
 }

 if (help) {
-if (end  argc) {
-vshError(ctl, _(extra argument '%s'. See --help.), argv[end]);
+if (optind  argc) {
+vshError(ctl, _(extra argument '%s'. See --help.), argv[optind]);
 exit(EXIT_FAILURE);
 }

@@ -11091,14 +11055,14 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
 exit(EXIT_SUCCESS);
 }

-if (argc  end) {
+if (argc  optind) {
 /* parse command */
 ctl-imode = FALSE;
-if (argc - end == 1) {
-vshDebug(ctl, 2, commands: \%s\\n, argv[end]);
-return vshCommandStringParse(ctl, argv[end]);
+if (argc - optind == 1) {
+vshDebug(ctl, 2, commands: \%s\\n, argv[optind]);
+return vshCommandStringParse(ctl, argv[optind]);
 } else {
-return vshCommandArgvParse(ctl, argc - end, argv + end);
+return vshCommandArgvParse(ctl, argc - optind, argv + optind);
 }
 }
 return TRUE;
-- 
1.7.2.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virsh: update comment about parsing

[Eric Blake]

* tools/virsh.c: Update comments to match patch series.
---

Noticed this one in reviewing the file once again; it's doc only, so
I'll apply it without an ACK once the rest of the series is in place.

 tools/virsh.c |   10 +-
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 4929f71..89c2e1e 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -100,18 +100,18 @@ typedef enum {
  *
  *command_line= command\n | command; command; ...
  *
- *command =keyword option data
+ *command =keyword option [--] data
  *
  *option  = bool_option | int_option | string_option
  *data= string
  *
  *bool_option = --optionname
- *int_option  = --optionname number
- *string_option   = --optionname string
+ *int_option  = --optionname number | --optionname=number
+ *string_option   = --optionname string | --optionname=string
  *
- *keyword = [a-zA-Z]
+ *keyword = [a-zA-Z][a-zA-Z-]*
  *number  = [0-9]+
- *string  = [^[:blank:]] | [[:alnum:]]$
+ *string  = ('[^']*'|([^\\]|\\.)*|([^ \t\n\\']|\\.))+
  *
  */

-- 
1.7.2.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Mac OS X: dyld: lazy symbol binding failed

[Mitchell Hashimoto]

Eric,

I've been getting this error lately from git (happening during `make`):

make[1]: *** No rule to make target `todo.pl', needed by `todo.html.in'.  Stop.
make: *** [distdir] Error 1

I haven't taken any time to look at it, but its caused by the
documentation task, obviously. I've been getting around this during
compile time with just doing make install which skips the doc task.

This is preventing me from testing my `make dist` tarball.

Mitchell

On Tue, Oct 12, 2010 at 3:04 PM, Eric Blake ebl...@redhat.com wrote:
 On 10/12/2010 03:56 PM, Mitchell Hashimoto wrote:

 I've been working with Justin, and we've been making some progress.
 However, I have another question for this list. As a follow-up to
 this, I realized that when I download the snapshots and just
 ./configure; make; make install then I get the lazy binding issue.
 However, if I go through the entire autogen process:

 ./autogen.sh
 make
 make install

 Then this issue goes away. Could this be indicative of a bug in the
 autotools input perhaps on generating the packages on machines which
 aren't Macs? How do I test the packaging myself (on a Mac) so that I
 can verify this theory?

 Not ringing any bells for me.

 What if you do:

 ./autogen.sh
 make
 make dist

 then expand that tarball to another directory, run ./configure and make, and
 compare the git tree with the tarball you just created?  Also, how does the
 snapshot compare to your tarball?

 Are you using git to create the snapshots (in which case I have no idea how
 the .gnulib submodule is being handled, if at all), or are they made from
 some 'make dist' cron job?

 --
 Eric Blake   ebl...@redhat.com    +1-801-349-2682
 Libvirt virtualization library http://libvirt.org


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [RFC PATCH] virsh: new echo command

[Eric Blake]

* tools/virsh.c (cmdEcho): New command.
(commands): Add it.
(vshCmdOptType): Add VSH_OT_ARGV.
(vshCmddefGetData): Special case new opt flag.
(vshCommandOptArgv): New function.
---

Not complete yet, but this shows what I'm thinking of.  Adding the
echo command has two benefits:

1. It will let me add unit tests for the recent virsh command line
improvements - echo back arbitrary strings to make sure quoting is as
desired.  This part works with what I have here, before I ran out of
time to finish this today.

2. Make it easier for a user on the command line to conver an
arbitrary string into something safe for shell evalution and/or XML
usage, by munging the input in a way that it can be reused in the
desired context.  Not yet implemented; hence the RFC.

It exploits the fact that -- is consumed as the end-of-options,
hence, there is no way for  to be recognized as a valid option
name, so the only way we can encounter VSH_OT_ARGV is via the
new argv handling, at which point we can handle all remaining
command line arguments.

 tools/virsh.c |   88 +++-
 1 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 89c2e1e..f361658 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -119,11 +119,12 @@ typedef enum {
  * vshCmdOptType - command option type
  */
 typedef enum {
-VSH_OT_NONE = 0,/* none */
-VSH_OT_BOOL,/* boolean option */
-VSH_OT_STRING,  /* string option */
-VSH_OT_INT, /* int option */
-VSH_OT_DATA /* string data (as non-option) */
+VSH_OT_NONE = 0, /* none */
+VSH_OT_BOOL, /* boolean option */
+VSH_OT_STRING,   /* string option */
+VSH_OT_INT,  /* int option */
+VSH_OT_DATA, /* string data (as non-option) */
+VSH_OT_ARGV  /* remaining arguments, opt-name should be  */
 } vshCmdOptType;

 /*
@@ -230,6 +231,7 @@ static char *vshCommandOptString(const vshCmd *cmd, const 
char *name,
 static long long vshCommandOptLongLong(const vshCmd *cmd, const char *name,
int *found);
 static int vshCommandOptBool(const vshCmd *cmd, const char *name);
+static char *vshCommandOptArgv(const vshCmd *cmd, int count);

 #define VSH_BYID (1  1)
 #define VSH_BYUUID   (1  2)
@@ -8917,6 +8919,54 @@ cmdPwd(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 #endif

 /*
+ * echo command
+ */
+static const vshCmdInfo info_echo[] = {
+{help, N_(echo arguments)},
+{desc, N_(Echo back arguments, possibly with quoting.)},
+{NULL, NULL}
+};
+
+static const vshCmdOptDef opts_echo[] = {
+{shell, VSH_OT_BOOL, 0, N_(escape for shell use)},
+{xml, VSH_OT_BOOL, 0, N_(escape for XML use)},
+{, VSH_OT_ARGV, 0, N_(arguments to echo)},
+{NULL, 0, 0, NULL}
+};
+
+/* Exists mainly for debugging virsh, but also handy for adding back
+ * quotes for later evaluation.
+ */
+static int
+cmdEcho (vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd)
+{
+bool shell = false;
+bool xml = false;
+int count = 0;
+char *arg;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (vshCommandOptBool(cmd, shell))
+shell = true;
+if (vshCommandOptBool(cmd, xml))
+xml = true;
+
+while ((arg = vshCommandOptArgv(cmd, count)) != NULL) {
+/* TODO - use buf */
+if (xml) {
+/* TODO - use virBufferEscapeString */
+}
+if (shell) {
+/* TODO - add '' and escape embedded ' */
+}
+vshPrint(ctl, %s%s, count ?   : , arg);
+count++;
+}
+
+return TRUE;
+}
+
+/*
  * edit command
  */
 static const vshCmdInfo info_edit[] = {
@@ -9545,6 +9595,7 @@ static const vshCmdDef commands[] = {
 {domxml-from-native, cmdDomXMLFromNative, opts_domxmlfromnative, 
info_domxmlfromnative},
 {domxml-to-native, cmdDomXMLToNative, opts_domxmltonative, 
info_domxmltonative},
 {dumpxml, cmdDumpXML, opts_dumpxml, info_dumpxml},
+{echo, cmdEcho, opts_echo, info_echo},
 {edit, cmdEdit, opts_edit, info_edit},
 {find-storage-pool-sources, cmdPoolDiscoverSources,
  opts_find_storage_pool_sources, info_find_storage_pool_sources},
@@ -9707,8 +9758,8 @@ vshCmddefGetData(const vshCmdDef * cmd, int data_ct)
 const vshCmdOptDef *opt;

 for (opt = cmd-opts; opt  opt-name; opt++) {
-if (opt-type == VSH_OT_DATA) {
-if (data_ct == 0)
+if (opt-type = VSH_OT_DATA) {
+if (data_ct == 0 || opt-type == VSH_OT_ARGV)
 return opt;
 else
 data_ct--;
@@ -9970,6 +10021,27 @@ vshCommandOptBool(const vshCmd *cmd, const char *name)
 return vshCommandOpt(cmd, name) ? TRUE : FALSE;
 }

+/*
+ * Returns the COUNT argv argument, or NULL after last argument.
+ *
+ * Requires that a VSH_OT_ARGV option with the name  be last in the
+ * list of supported options in CMD-def-opts.
+ */
+static 

Re: [libvirt] KVM Windows guest: add hard drive

[Mihamina Rakotomandimby]

 acath...@redhat.com :
 Are you looking for a new driver to appear in 'my computer'

Yes, that is what I expected.

 because it wouldn't appear there until you format it. Does it show
 up in device manager?

I'll try to see in the device manager and see if I can ask for
formatting from somewhere.

-- 

   Architecte Informatique chez Blueline/Gulfsat:
Administration Systeme, Recherche  Developpement
+261 34 56 000 19

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 04/13] XML parsing for memory tunables

[Nikunj A. Dadhania]

On Tue, 12 Oct 2010 16:54:39 +0200, Daniel Veillard veill...@redhat.com wrote:
 On Fri, Oct 08, 2010 at 05:45:23PM +0530, Nikunj A. Dadhania wrote:
  From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
  
  Adding parsing code for memory tunables in the domain xml file
  
  v4:
  * Add memtune in tests/qemuxml2xmltest.c
  * Fix: insert memtune element only when any of them is set
  
  v2:
  + Fix typo min_guarantee
 
   The patch is fine except the usual space and tabs mixups and the fact
 that a number of drivers still needed to be converted to the change
 of the definition structure. grep -- -memory src/*/* isn't that
 hard and would have shown that even the driver for your own IBM Phyp
 hardware failed to compile after your patch !!
 
   anyway once cleaned up the patch makes sensei, ACK, but please use
 make syntax-check and do not configure out drivers when you are
 developping patches,
 
Thanks Daniel,

Did not know about the make syntax-check. And as you guessed, I did not
compile it for other drivers, just went out of my mind, I will take care next
time.

Regards,
Nikunj

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 07/13] Implement driver interface domainGetMemoryParamters for QEmu

[Nikunj A. Dadhania]

On Tue, 12 Oct 2010 17:51:54 +0200, Daniel Veillard veill...@redhat.com wrote:
 On Fri, Oct 08, 2010 at 05:45:55PM +0530, Nikunj A. Dadhania wrote:
  From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
  
  V4:
  * prototype change: add unsigned int flags
  
  Driver interface for getting memory parameters, eg. hard_limit, soft_limit 
  and
  swap_hard_limit.
 
  +qemuReportError(VIR_ERR_INVALID_ARG,
  +%s, _(Invalid parameter count));
  +goto cleanup;
  +}
 
   okay, this mean the application must always call with 0 first to get
   the exact value or this will break, fine but probably need to be made
   more clear from the description in libvirt.c  TODO
 
Sure, I will take care of updating the api desc in libvirt.c, I haven't used
word always there.

  +if (virCgroupForDomain(driver-cgroup, vm-def-name, group, 0) != 0) 
  {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +_(cannot find cgroup for domain %s), 
  vm-def-name);
  +goto cleanup;
  +}
  +
  +for (i = 0; i  *nparams; i++) {
  +virMemoryParameterPtr param = params[i];
  +val = 0;
  +param-value.ul = 0;
  +param-type = VIR_DOMAIN_MEMORY_FIELD_ULLONG;
  +
  +switch(i) {
  +case 0: /* fill memory hard limit here */
  +rc = virCgroupGetMemoryHardLimit(group, val);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to get memory hard limit));
  +continue;
  +}
  +if (virStrcpyStatic(param-field, 
  VIR_DOMAIN_MEMORY_HARD_LIMIT) == NULL) {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +%s, _(Field memory hard limit too long 
  for destination));
  +continue;
  +}
  +param-value.ul = val;
  +break;
  +
  +case 1: /* fill memory soft limit here */
  +rc = virCgroupGetMemorySoftLimit(group, val);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to get memory soft limit));
  +continue;
  +}
  +if (virStrcpyStatic(param-field, 
  VIR_DOMAIN_MEMORY_SOFT_LIMIT) == NULL) {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +%s, _(Field memory soft limit too long 
  for destination));
  +continue;
  +}
  +param-value.ul = val;
  +break;
  +   
  +case 2: /* fill swap hard limit here */
  +rc = virCgroupGetSwapHardLimit(group, val);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to get swap hard limit));
  +continue;
  +}
  +if (virStrcpyStatic(param-field, VIR_DOMAIN_SWAP_HARD_LIMIT) 
  == NULL) {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +%s, _(Field swap hard limit too long 
  for destination));
  +continue;
  +}
  +param-value.ul = val;
  +break;
  +
  +default:
  +break;
  +/* should not hit here */
  +}
  +}
 
   Okay, I'm not sure we actually need a loop here, but it may help
   refactoring...
I guess this is related to my previous thinking, if nparams 
QEMU_NB_MEM_PARAM, fill only till nparams and return. But with the change of
the logic, I think loop may not be required now.

 I'm still having a problem with the code ignoring any error occuring in
 the loop, and fixing this in the same way. If there is an error the
 application *must* learn about it instead of trusting uninitialized
 memory as being data !
 Maybe a memset is in order actually before entering that loop to avoid
 edge case problems... TODO too

By TODO you mean the error handling, right?  

I am taking care of setting the values to zero currently, and it does not tell
the application whether to use this value or not.  One option could be adding
VIR_DOMAIN_MEMORY_INVALID in virMemoryParameterType and setting it in the
beginning of the loop. Comments?

Nikunj

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 10/13] Implement driver interface domainSetMemoryParamters for LXC

[Nikunj A. Dadhania]

On Tue, 12 Oct 2010 18:32:19 +0200, Daniel Veillard veill...@redhat.com wrote:
 On Fri, Oct 08, 2010 at 05:46:28PM +0530, Nikunj A. Dadhania wrote:
  From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
  
  Add support in the lxc driver for various memory controllable parameters
  
  v4:
  + prototype change: add unsigned int flags
  
  v2:
  + Use #define string constants for hard_limit, etc
  + fix typo: min_guarantee
  
  Acked-by: Daniel P. Berrange berra...@redhat.com
  Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
[...]
  +if (vm == NULL) {
  +char uuidstr[VIR_UUID_STRING_BUFLEN];
  +virUUIDFormat(dom-uuid, uuidstr);
  +lxcError(VIR_ERR_NO_DOMAIN,
  + _(No domain with matching uuid '%s'), uuidstr);
  +goto cleanup;
  +}
 
   Hum, the qemu driver was reporting
 
 if (vm == NULL) {
 qemuReportError(VIR_ERR_INTERNAL_ERROR,
 _(No such domain %s), dom-uuid);
 goto cleanup;
 }
 
 the 2 should be harmonized I guess, but since the LXC reporting is better
 I left this as a TODO, seems that's a more general cleanup needed between
 drivers.
 
Let me look at this and I will provide a patch.

 
  +for (i = 0; i  nparams; i++) {
  +virMemoryParameterPtr param = params[i];
  +
  +if (STREQ(param-field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) {
  +int rc;
  +if (param-type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
  +lxcError(VIR_ERR_INVALID_ARG, %s,
  + _(invalid type for memory hard_limit tunable, 
  expected a 'ullong'));
  +continue;
  +}
  +
  +rc = virCgroupSetMemoryHardLimit(cgroup, params[i].value.ul);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to set memory hard_limit 
  tunable));
  +}
  +} else if (STREQ(param-field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) {
  +int rc;
  +if (param-type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
  +lxcError(VIR_ERR_INVALID_ARG, %s,
  + _(invalid type for memory soft_limit tunable, 
  expected a 'ullong'));
  +continue;
  +}
  +
  +rc = virCgroupSetMemorySoftLimit(cgroup, params[i].value.ul);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to set memory soft_limit 
  tunable));
  +}
  +} else if (STREQ(param-field, VIR_DOMAIN_SWAP_HARD_LIMIT)) {
  +int rc;
  +if (param-type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
  +lxcError(VIR_ERR_INVALID_ARG, %s,
  + _(invalid type for swap_hard_limit tunable, 
  expected a 'ullong'));
  +continue;
  +}
  +
  +rc = virCgroupSetSwapHardLimit(cgroup, params[i].value.ul);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to set swap_hard_limit 
  tunable));
  +}
  +} else if (STREQ(param-field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE)) {
  +lxcError(VIR_ERR_INVALID_ARG,
  + _(Memory tunable `%s' not implemented), 
  param-field);
  +} else {
  +lxcError(VIR_ERR_INVALID_ARG,
  + _(Parameter `%s' not supported), param-field);
  +}
  +}
  +ret = 0;
 
   Same problem of error reporting as in the QEmu driver, I moved ret = 0;
 before the loop and et ret = -1; on all errors !

One clarification:
Will it return error back immediately if an error occurs?
Or will it try setting all of them one by one and if anyone of them succeed,
success is returned.

  +cleanup:
  +if (cgroup)
  +virCgroupFree(cgroup);
  +if (vm)
  +virDomainObjUnlock(vm);
  +lxcDriverUnlock(driver);
  +return ret;
  +}
  +
 
  After those modifications, ACK, applied to my tree,
 
Thanks

Nikunj

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 13/13] Remote protocol changes and implements virDomainSet/GetMemoryParameters

[Nikunj A. Dadhania]

On Tue, 12 Oct 2010 19:27:05 +0200, Daniel Veillard veill...@redhat.com wrote:
 On Fri, Oct 08, 2010 at 05:47:03PM +0530, Nikunj A. Dadhania wrote:
  From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
  
  v4:
  * prototype change: add unsigned int flags, regenerate files
  
  v3:
  * Squased all the remote driver changes to one single big patch and
auto-generated is around 40%
  * Implements domainSetMemoryParameters and domainGetMemoryParameters for 
  remote
driver
  daemon/remote.c
  src/remote/remote_driver.c
  
  * Auto generate the files using rpcgen and helper scripts in daemon/ 
  directory
  src/remote/remote_protocol.x
  daemon/remote_dispatch_args.h
  daemon/remote_dispatch_prototypes.h
  daemon/remote_dispatch_ret.h
  daemon/remote_dispatch_table.h
  src/remote/remote_protocol.c
  src/remote/remote_protocol.h
  
  Acked-by: Daniel P. Berrange berra...@redhat.com
  Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 [...]
  +nparams = args-params.params_len;
  +nparams = args-flags;
 
   obvious bug: flags = args-flags;
Oops :) - CP

 
 That reminds me that I didn't see flags being checked against 0 on the
 QEmu and LXC drivers, we should check them !
 
I will take care of this.

  +if (nparams  REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) {
  +remoteDispatchFormatError (rerr, %s, _(nparams too large));
  +return -1;
 
   I did a lot of reformating and code cleanups in the non-generated
   files, to try to keep the source readable on a 80 columns editor.

Thanks,
 
   With that done, I think I can ACK the whole serie, and pushed it, but
 there is still a number of small TODOs for which I would appreciate
 patches,
 
   thanks a lot !
 
Thanks a lot to you and Daniel Berrange. Both of you have spend a lot of time
on reviewing and on top of that fixing the patches as well.

Regards,
Nikunj

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 04/13] XML parsing for memory tunables

[Nikunj A. Dadhania]

On Tue, 12 Oct 2010 19:19:21 +0530, Balbir Singh bal...@linux.vnet.ibm.com 
wrote:
 * Nikunj A. Dadhania nik...@linux.vnet.ibm.com [2010-10-08 14:43:34]:
 
  On Fri, 8 Oct 2010 14:10:53 +0530, Balbir Singh bal...@linux.vnet.ibm.com 
  wrote:
   * Nikunj A. Dadhania nik...@linux.vnet.ibm.com [2010-10-08 12:00:44]:
   
On Thu, 7 Oct 2010 12:49:29 +0100, Daniel P. Berrange 
berra...@redhat.com wrote:
 On Mon, Oct 04, 2010 at 12:47:22PM +0530, Nikunj A. Dadhania wrote:
  On Mon, 4 Oct 2010 12:16:42 +0530, Balbir Singh 
  bal...@linux.vnet.ibm.com wrote:
   * Nikunj A. Dadhania nik...@linux.vnet.ibm.com [2010-09-28 
   15:26:30]:
snip
+unsigned long hard_limit;
+unsigned long soft_limit;
+unsigned long min_guarantee;
+unsigned long swap_hard_limit;
   
   The hard_limit, soft_limit, swap_hard_limit are s64 and the value 
   is
   in bytes. What is the unit supported in this implementation?
 
 Actually if libvirt is built on 32bit these aren't big enough - make
 them into 'unsigned long long' data types I reckon.
 
I was thinking that as we are having the unit of KB, we would be able to
represent 2^42 bytes of memory limit, ie. 4 Terabytes. Won't this 
suffice in
case of 32bit?
   
   
   How would you represent -1 (2^63 -1) as unlimited or max limit we use
   today? 
   
  I think I have answered this question in the thread: this is specific to
  cgroup that -1 means unlimited, this may not be true for other HVs.
 
 OK, so how do we handle unlimited values in general?
 
At present, API does not have a way to do this. We have added a flag in the
API, let me think of some way I could use it for such setting unlimited
values.

Regards
Nikunj

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list