Ian Campbell wrote:
> ... and consolidate the cmdline/extra/root parsing to facilitate doing
> so.
>
> The logic is the same as xl's parse_cmdline from the current xen.git master
> branch (e6f0e099d2c17de47fd86e817b1998db903cab61).
>
> In order to introduce a use of VIR_WARN for logging I had to add
> virerror.h and VIR_LOG_INIT.
>
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
> NB: I was unsure if I was supposed to change VIR_FROM_NONE into
> VIR_FROM_XEN, so I didn't (but will on advice).
It seems some of the VIR_FROM_ needs cleanup throughout the various Xen-related
files. We already have VIR_FROM_SEXPR and VIR_FROM_XENXM.
src/xenconfig/xen_sxpr.c should use the former, src/xenconfig/xen_xm.c the
latter. And given the pattern, I think we should introduce VIR_FROM_XENXL to
cover xl.cfg related code. I can take care of this.
>
> v2: Use VIR_INFO (adding necessary infra)
> Don't initialise things to NULL when there is no need.
> ---
> src/xenconfig/xen_xl.c | 66 +++++++++++++++++++++++++++++++-------------------
> 1 file changed, 41 insertions(+), 25 deletions(-)
>
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 91cdff6..3d820cc 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -27,6 +27,7 @@
>
> #include "virconf.h"
> #include "virerror.h"
> +#include "virlog.h"
> #include "domain_conf.h"
> #include "viralloc.h"
> #include "virstring.h"
> @@ -35,6 +36,8 @@
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> +VIR_LOG_INIT("xen.xen_xl");
> +
> /*
> * Xen provides a libxl utility library, with several useful functions,
> * specifically xlu_disk_parse for parsing xl disk config strings.
> @@ -58,11 +61,46 @@ extern int xlu_disk_parse(XLU_Config *cfg,
> libxl_device_disk *disk);
> #endif
>
> +static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
> +{
> + char *cmdline;
One too many initializers removed :-).
> + const char *root, *extra, *buf;
> +
> + if (xenConfigGetString(conf, "cmdline", &buf, NULL) < 0)
> + return -1;
> +
> + if (xenConfigGetString(conf, "root", &root, NULL) < 0)
> + return -1;
> +
> + if (xenConfigGetString(conf, "extra", &extra, NULL) < 0)
> + return -1;
> +
> + if (buf) {
> + if (VIR_STRDUP(cmdline, buf) < 0)
> + return -1;
> + if (root || extra)
> + VIR_WARN("ignoring root= and extra= in favour of cmdline=");
> + } else {
> + if (root && extra) {
> + if (virAsprintf(&cmdline, "root=%s %s", root, extra) < 0)
> + return -1;
> + } else if (root) {
> + if (virAsprintf(&cmdline, "root=%s", root) < 0)
> + return -1;
> + } else if (extra) {
> + if (VIR_STRDUP(cmdline, extra) < 0)
> + return -1;
> + }
> + }
> +
> + *r_cmdline = cmdline;
If none of cmdline, extra, or root are set in the config, def->os.cmdline gets
set to garbage. xlconfigtest explodes when running 'make check'.
Sorry for not mentioning it in my previous review, but we should add a test for
cmdline, root, and extra.
Regards,
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list