On Wed, May 13, 2009 at 03:40:54PM +0100, Daniel P. Berrange wrote:
> This provides the QEMU driver implementation which is able to convert
> from QEMU argv into domain XML. This is alot of hard code, because we
> have to parse and interpret arbitrary QEMU args and had no existing
> code doing this. This is also actually the single most useful feature
> of this patchset, and the original motivation. With this available, it
> makes it very easy for people using QEMU to switch over to using libvirt
> for management
[...]
> + /* Iterate over string, splitting on sequences of ' ' */
> + while (curr && *curr != '\0') {
> + char *arg;
> + const char *next = strchr(curr, ' ');
> + if (!next)
> + next = strchr(curr, '\n');
> +
> + if (next)
> + arg = strndup(curr, next-curr);
> + else
> + arg = strdup(curr);
> +
> + if (!arg)
> + goto no_memory;
> +
> + if (VIR_REALLOC_N(arglist, argcount+1) < 0) {
> + VIR_FREE(arg);
> + goto no_memory;
> + }
each time you use realloc in a loop god kill some innocent electrons
Seriously realloc used to be really really slow on some systems but heh
> + arglist[argcount++] = arg;
> +
> + while (next && c_isspace(*next))
> + next++;
> +
> + curr = next;
> + }
> +
> + /* Iterate over list of args, finding first arg not containining
> + * and '=' character (eg, skip over env vars FOO=bar) */
s/and /the /
> + for (envend = 0 ; envend < argcount && strchr(arglist[envend], '=') ; envend++)
can we parenthesize a bit ?
(envend < argcount) && (strchr(arglist[envend], '='))
and break the long line ?
> + ; /* nada */
> +
> + /* Copy the list of env vars */
> + if (envend > 0) {
> + if (VIR_REALLOC_N(progenv, envend+1) < 0)
> + goto no_memory;
> + for (i = 0 ; i < envend ; i++) {
> + progenv[i] = arglist[i];
> + }
> + progenv[i] = NULL;
> + }
> +
> + /* Copy the list of argv */
> + if (VIR_REALLOC_N(progargv, argcount-envend + 1) < 0)
> + goto no_memory;
> + for (i = envend ; i < argcount ; i++)
> + progargv[i-envend] = arglist[i];
> + progargv[i-envend] = NULL;
> +
> + VIR_FREE(arglist);
> +
> + *retenv = progenv;
> + *retargv = progargv;
> +
> + return 0;
> +
> +no_memory:
> + for (i = 0 ; progenv && progenv[i] ; i++)
> + VIR_FREE(progenv);
bug VIR_FREE(progenv[i]);
if (progenv) could be moved out of the loop too but I'm so old
fashionned...
Reminds me my father complaining that those computer guys were lazy
bastards because those Cray compilers could not vectorize his code
properly and he had to do it by hand !
> + VIR_FREE(progenv);
> + for (i = 0 ; i < argcount ; i++)
> + VIR_FREE(arglist[i]);
> + VIR_FREE(arglist);
> + return -1;
> +}
[...]
> +/*
> + * Takes a string containing a set of key=value,key=value,key...
> + * parameters and splits them up, returning two arrays with
> + * the individual keys and values
> + */
> +static int
> +qemuParseCommandLineKeywords(virConnectPtr conn,
> + const char *str,
> + char ***retkeywords,
> + char ***retvalues)
typedef char **strtable;
and
strtable *retkeywords, strtable *retvalues
but that would kill the fun of reading it
[...]
> + while (start) {
[...]
> + if (VIR_REALLOC_N(keywords, nkeywords+1) < 0 ||
> + VIR_REALLOC_N(values, nkeywords+1) < 0) {
> + VIR_FREE(keyword);
> + VIR_FREE(value);
> + goto no_memory;
> + }
worse than a realloc in a loop, 2 reallocs in a loop ...
Maybe we should do something about it, no ? Some nice macro
which would define the array array##max array##cur and
and allocation macro which would realloc only when array##cur
reaches array##max ?
[...]
> +/*
> + * Tries to parse new style QEMU -drive args
Try only ?
> + * eg
> + * -drive file=/dev/HostVG/VirtData1,if=ide,index=1 -
Actually the parsing routine is not that bad...
> +/*
> + * Tries to parse a QEMU -net backend argument. Gets given
> + * a list of all known -net frontend arguments to try and
> + * match up against. Horribly complicated stuff
> + */
> +static virDomainNetDefPtr
> +qemuParseCommandLineNet(virConnectPtr conn,
> + const char *val,
> + int nnics,
> + const char **nics)
> +{
Agreed seems we are playing heuristics here...
> +/*
> + * Tries to parse a QEMU PCI device
actually that one is USB :-)
> + */
> +static virDomainHostdevDefPtr
> +qemuParseCommandLineUSB(virConnectPtr conn,
> + const char *val)
> +{
> + } else {
> + if (virStrToLong_i(start, &end, 10, &first) < 0 || !end || *end != '.') {
> + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("cannot extract PCI device bus '%s'"), val);
> + VIR_FREE(def);
> + goto cleanup;
> + }
> + start = end + 1;
> + if (virStrToLong_i(start, NULL, 10, &second) < 0) {
Hum, the base address is really expected to be given in base 10 ? I
would assume it's base 16 instead, right ?
> + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("cannot extract PCI device address '%s'"), val);
> + VIR_FREE(def);
> + goto cleanup;
> + }
> + }
[...]
> +virDomainDefPtr qemuParseCommandLine(virConnectPtr conn,
> + const char **progenv,
> + const char **progargv)
> +{
[...]
> + if (!def->os.arch)
> + goto no_memory;
> +
> +#define WANT_VALUE() \
> + const char *val = progargv[++i]; \
> + if (!val) { \
> + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, \
> + _("missing value for %s argument"), arg); \
> + goto error; \
> + }
Urgh, I always feel that macro definition within function are an abuse
of layering, sure it's preprocessor candy but that won't bring any
safety ....
> + /* One initial loop to get list of NICs, so we
> + * can correlate them later */
> + for (i = 1 ; progargv[i] ; i++) {
> + const char *arg = progargv[i];
> + /* Make sure we have a single - for all options to
> + simplify next logic */
> + if (STRPREFIX(arg, "--"))
> + arg++;
> +
> + if (STREQ(arg, "-net")) {
> + WANT_VALUE();
> + if (STRPREFIX(val, "nic")) {
> + if (VIR_REALLOC_N(nics, nnics+1) < 0)
> + goto no_memory;
> + nics[nnics++] = val;
> + }
> + }
> + }
yes the network stuff is really ugly !
[...]
> + } else {
> +#if 1
> + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("unknown argument '%s'"), arg);
> + goto error;
> +#endif
I would keep it in no matter what
> + }
> + }
> +
> +#undef WANT_VALUE
....
[...]
> --- a/src/vbox/vbox_tmpl.c Wed May 13 10:02:15 2009 -0400
> +++ b/src/vbox/vbox_tmpl.c Wed May 13 10:02:17 2009 -0400
> @@ -1803,7 +1803,7 @@ static char *vboxDomainDumpXML(virDomain
> if (audioController == AudioControllerType_SB16) {
> def->sounds[0]->model = VIR_DOMAIN_SOUND_MODEL_SB16;
> } else if (audioController == AudioControllerType_AC97) {
> - def->sounds[0]->model = VIR_DOMAIN_SOUND_MODEL_ES97;
> + def->sounds[0]->model = VIR_DOMAIN_SOUND_MODEL_AC97;
> }
> } else {
> VIR_FREE(def->sounds);
> @@ -2767,7 +2767,7 @@ static virDomainPtr vboxDomainDefineXML(
> if (NS_SUCCEEDED(rc)) {
> if (def->sounds[0]->model == VIR_DOMAIN_SOUND_MODEL_SB16) {
> audioAdapter->vtbl->SetAudioController(audioAdapter, AudioControllerType_SB16);
> - } else if (def->sounds[0]->model == VIR_DOMAIN_SOUND_MODEL_ES97) {
> + } else if (def->sounds[0]->model == VIR_DOMAIN_SOUND_MODEL_AC97) {
> audioAdapter->vtbl->SetAudioController(audioAdapter, AudioControllerType_AC97);
> }
> }
The renaming of the ES97 into AC97 is IMHO orthogonal to this patch
Conclusion is that it's time qemu switch out of crazy arg formats and get
a decent config format instead ! This code could be a first step toward
sanity, but good luck arguing with them upstream !
ACK, it's insane !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list