Re: [libvirt] [PATCH 2/2] qemu: Reject unsupported tuning in session mode

On 03/03/2014 10:21 AM, Martin Kletzander wrote: > When domain is started with setting that cannot be done, i.e. those > that require cgroups, there is no error reported and it succeeds > without any message whatsoever. > > When setting with API, virsh, an error is reported, but only due to > the fact that no cgroups are mounted (priv->cgroup == NULL). > > Given the above it seems reasonable to reject such unsupported > settings. > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > > Notes: > Few questions came to my mind while writing the commit message: > > 1) Would helper function (macro) be preferred so the code looks > cleaner? What macro do you have in mind?

> > 2) Do we want to allow reading of some of these settings through an > API? This would however require our cgroup handling to be > reworked. > > 3) Would new error type (for session-unsupported settings) be any > good or it doesn't make sense to create one just for these added > messages plus few older ones (just guessing the amount)? Looks like your use of VIR_ERR_CONFIG_UNSUPPORTED was reasonable. > + if (!cfg->privileged) { > + /* If we have no cgroups than we can have no tunings that > + * require them */ > + > + if (def->mem.hard_limit || def->mem.soft_limit || > + def->mem.min_guarantee || def->mem.swap_hard_limit) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Memory tuning is not available in session mode")); > + goto error; > + } > + Or maybe we should think about some day adding a daemon that accepts RPC commands for manipulating cgroups on behalf of a session client. But that would be a later and bigger patch.

Seems reasonable to me, but it might be nice to also get Dan's opinion on this one.