On Tue, Mar 04, 2014 at 02:04:05PM -0700, Eric Blake wrote: > On 03/04/2014 07:15 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. > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1023366 > > > > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> > > --- > > > > @@ -7443,6 +7445,13 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom, > > if (virDomainGetSchedulerTypeEnsureACL(dom->conn, vm->def) < 0) > > goto cleanup; > > > > + cfg = virQEMUDriverGetConfig(driver); > > + if (!cfg->privileged) { > > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > > + _("CPU tuning is not available in session mode")); > > + goto cleanup; > > + } > > + > > /* Domain not running, thus no cgroups - return defaults */ > > if (!virDomainObjIsActive(vm)) { > > I can understand failing the Set commands if we can't change things; but > since we have a fallback for the Get command even for an offline domain, > shouldn't we stick to returning the defaults rather than erroring out? > Particularly for SchedulerType, we can allow the Get method, but the reading will still fail as there are no cgroups set up. If we rework our cgroups code for non-privileged daemons, we will have to check the cgroup in which the domain belongs to, which will be the same as other user processes, and doe to that fact the settings won't apply only to that domain, but for everything in that cgroup; that will make the values less useful (e.g. maximum used memory cannot be guaranteed since the cgroup has more tasks inside than only qemu). Our cgroup path assembly functions will then need to differ based on whether it is non-privileged or not. If that's really needed, I'll rework it that way (even though it'll probably need to get changed back when user cgroups will be user-editable), but until then, could we make the error reported now: $ virsh -c qemu:///session schedinfo dummy Scheduler : Unknown error: Requested operation is not valid: cgroup CPU controller is not mounted changed into: $ virsh -c qemu:///session schedinfo dummy Scheduler : Unknown error: Operation not supported: CPU tuning is not available in session mode ??? // I should've added that info into the commit message, I just forgot. Martin > Weak ACK; I'd still wait for Dan to weigh in. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list