From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Fehlig Subject: Re: [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files Date: Wed, 20 Jan 2016 11:16:15 -0700 Message-ID: <569FCEEF.4060809@suse.com> References: <1453291544-29323-1-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1453291544-29323-1-git-send-email-ian.campbell@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: libvir-list@redhat.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > --- > 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