Re: [libvirt] [PATCH] Return a suitable error message if we can't find a matching emulator
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
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
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
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
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
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
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
-- 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
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
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
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
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
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
* 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
* 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
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
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
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
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.
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
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
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
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
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
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
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
- 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
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
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
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
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
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
* 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
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
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
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
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
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
* 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
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
* 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
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
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
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
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
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
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