On Thu, Jul 18, 2013 at 09:15:28PM -0600, Eric Blake wrote:

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:

...

From: "Daniel P. Berrange" <berrange@redhat.com>

Instead of returning raw errno values, report full libvirt errors in virCgroupNew* functions.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/lxc/lxc_cgroup.c | 83 +++----- src/lxc/lxc_container.c | 6 +- src/lxc/lxc_fuse.c | 6 +- src/qemu/qemu_cgroup.c | 87 +++----- src/qemu/qemu_driver.c | 76 ++----- src/util/vircgroup.c | 515 ++++++++++++++++++++++++----------------------- src/util/virerror.c | 46 +++++ src/util/virerror.h | 4 + tests/vircgrouptest.c | 44 +++- 10 files changed, 431 insertions(+), 437 deletions(-)

Big, but in the right direction.

...

+++ b/src/libvirt_private.syms @@ -1302,6 +1302,7 @@ ebtablesRemoveForwardAllowIn; # util/virerror.h virDispatchError; virErrorInitialize; +virLastErrorIsSystemErrno;

Is it worth splitting this into two patches - one that addes the new virLastErrorIsSystemErrno, the other that fixes cgroup to use it and adjusts cgroup callers?

Yep, I'll split out the virerror.{c,h} additions.

...

+++ b/src/lxc/lxc_cgroup.c

[3 start]

...

- rc = virCgroupNewDomainDriver(parent, - def->name, - true, - &cgroup); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to create cgroup for %s"), - def->name); + if (virCgroupNewDomainDriver(parent, + def->name, + true, + &cgroup) < 0) goto cleanup;

A lot more compact. Is it worth trying to unwrap any lines if the arguments would fit in 80 columns?

There are a fair few more cgroups changes to come, so I'm preferring not to try to change too much at least time.

...

+++ b/src/lxc/lxc_fuse.c @@ -139,8 +139,10 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, virBuffer buffer = VIR_BUFFER_INITIALIZER; virBufferPtr new_meminfo = &buffer;

- if ((res = virLXCCgroupGetMeminfo(&meminfo)) < 0) - return res; + if (virLXCCgroupGetMeminfo(&meminfo) < 0) { + virErrorSetErrnoFromLastError(); + return -errno;

Ah, I see how this works. But now I need to take a side trip to check something. [end 3]

[5 start] and I'm back. Luckily, you only called virErrorSetErrnoFromLastError() when you KNOW there is a last error in place.

Actually the point of virErrorSetErrnoFromLastError is that it will always ensure that an errno is set - it'll default to EIO if there was no error object raised. That way if virLXCCgroupGetMeminfo forgot to raise an error, we're still going to report EIO back to the caller of lxcProcReadMeminfo, which is critical in this error path.

...

+ if (virCgroupNewPartition(vm->def->resource->partition, + STREQ(vm->def->resource->partition, "/machine"), + cfg->cgroupControllers, + &parent) < 0) { + virErrorPtr err = virGetLastError(); + if (err && err->code == VIR_ERR_SYSTEM_ERROR && + (err->int1 == ENXIO || + err->int1 == EPERM || + err->int1 == EACCES)) { /* No cgroups mounts == success */

Hmm, you were unable to use virLastErrorIsSystemErrno, but had to inline the checks yourself. Does that mean we didn't get quite the right interface for how we will be using it?

No, it means I added virLastErrorIsSystemErrno only recently after I had done this change & didn't think to go back and use it here too :-)

...

VIR_DEBUG("No cgroups present/configured/accessible, ignoring error"); + virResetLastError();

Will people complain about log pollution? IIRC, even if we reset the last error, the mere creation of the error puts something in the log before we reset, compared the the pre-patch code that never triggered an error. How often will this reset be hit on a system without cgroups but running lots of domains?

It'll be hit on every VM start. We could optimize a little bit so that we check for existance of /proc/cgroups, and are a complete no-op in that case. Then this would only spam logs if cgroups was enabled in the kernel, but not mounted on the host, which is probably a reasonable tradeoff.

...

+ if (virCgroupNewDriver("qemu", + true, + cfg->cgroupControllers, + &parent) < 0) { + virErrorPtr err = virGetLastError(); + if (err && err->code == VIR_ERR_SYSTEM_ERROR && + (err->int1 == ENXIO || + err->int1 == EPERM || + err->int1 == EACCES)) { /* No cgroups mounts == success */ VIR_DEBUG("No cgroups present/configured/accessible, ignoring error"); + virResetLastError();

Hmm, we have the same inlined check twice - again an argument that we ought to have a helper function to make this easier. Maybe call it virCgroupErrorIgnorable, and have that function automatically reset the last error when it returns true?

Yep, could do.

...

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 9dfe98d..12ace2e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c

[2 start]

I'm checking what you did; but I did not check whether you missed any conversions. If we find more, we can do them as a followup patch.

...

@@ -646,8 +650,9 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group) inherit_values[i], &value); if (rc != 0) {

rc is set by virCgroupGetValueStr, ...

...

- VIR_ERROR(_("Failed to get %s %d"), inherit_values[i], rc); - break; + virReportSystemError(-rc, + _("Failed to get '%s'"), inherit_values[i]);

...and you are now reporting it as an errno value. I guess that still works, because you didn't convert virCgroupGetValueStr, but will it bite us later? I guess it goes back to your question of whether ANY libvirt interface should return -errno, or whether we should convert even more to just use virError.

The 3rd patch in this series will remove the virReportSystemError call entirely, at the same time that virCgrouPGetValueStr stops returning errno values. So we'll be ok here.

...

@@ -795,51 +804,41 @@ static int virCgroupMakeGroup(virCgroupPtr parent, * @parent is NULL, then the placement of the current * process is used. * + * Returns 0 on success, -1 on error */ static int virCgroupNew(const char *path, virCgroupPtr parent, int controllers, virCgroupPtr *group) { - int rc = 0; - char *typpath = NULL;

Huh,...

...

- VIR_DEBUG("parent=%p path=%s controllers=%d", parent, path, controllers); *group = NULL;

- if (VIR_ALLOC((*group)) != 0) { - rc = -ENOMEM; - goto err; - } + if (VIR_ALLOC((*group)) < 0) + goto error;

if (path[0] == '/' || !parent) { - if (VIR_STRDUP_QUIET((*group)->path, path) < 0) { - rc = -ENOMEM; - goto err; - } + if (VIR_STRDUP((*group)->path, path) < 0) + goto error; } else { if (virAsprintf(&(*group)->path, "%s%s%s", parent->path, STREQ(parent->path, "") ? "" : "/", - path) < 0) { - rc = -ENOMEM; - goto err; - } + path) < 0) + goto error; }

- rc = virCgroupDetect(*group, controllers, path, parent); - if (rc < 0) - goto err; + if (virCgroupDetect(*group, controllers, path, parent) < 0) + goto error;

- return rc; -err: + return 0; + +error: virCgroupFree(group); *group = NULL;

- VIR_FREE(typpath);

...looks like typpath was a dead variable. Wonder why Coverity didn't flag it as dead?

Possibly because VIR_FREE expands to virFree(&typepath); and it hasn't figured out that virFree doesn't have side-effects on the rest of the system state.

...

@@ -1152,7 +1153,8 @@ static int virCgroupPartitionNeedsEscaping(const char *path) }

if (ferror(fp)) { - ret = -EIO; + virReportSystemError(errno, "%s", + _("Error while reading /proc/cgroups"));

errno after ferror() is not always the world's most reliable content; but I guess it's better than a blanket EIO.

...

@@ -2454,8 +2463,10 @@ static int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHas

VIR_DEBUG("Process subdir %s", ent->d_name);

- if ((rc = virCgroupNew(ent->d_name, group, -1, &subgroup)) != 0) + if (virCgroupNew(ent->d_name, group, -1, &subgroup) < 0) { + rc = -EIO;

Do we blindly want -EIO here, or should we try virErrorSetErrnoFromLastError()?

Well this goes away in the next patch, so it doesn't matter in the end.

...

goto cleanup; + }

if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids, true)) < 0) goto cleanup;

[end 2]

...

diff --git a/src/util/virerror.c b/src/util/virerror.c index ce3ab85..de4479e 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c

[1 start]

...

@@ -1397,3 +1397,49 @@ void virSetErrorLogPriorityFunc(virErrorLogPriorityFunc func) { virErrorLogPriorityFilter = func; } + + +/** + * virErrorSetErrnoFromLastError: + * + * If the last error had a code of VIR_ERR_SYSTEM_ERROR + * then set errno to the value saved in the error object. + * + * If the last error had a code of VIR_ERR_NO_MEMORY + * then set errno to ENOMEM

Possible other mappings to consider:

VIR_ERR_INVALID_{CONN,DOMAIN,ARG} to EINVAL?

VIR_ERR_OPERATION_DENIED to EPERM?

VIR_ERR_OPERATION_TIMEOUT to ETIMEDOUT?

VIR_ERR_OVERFLOW to EOVERFLOW or ERANGE?

I was only really considering error codes used by the cgroups code here. The only reason we need this at all is to simplify integration with FUSE & I don't expect we'll use it anywhere else in libvirt, so lets keep it simple.

...

+ * + * Otherwise set errno to EIO.

Fair enough, given we don't know much more about it.

...

+ */ +void virErrorSetErrnoFromLastError(void) +{ + virErrorPtr err = virGetLastError(); + if (err && err->code == VIR_ERR_SYSTEM_ERROR) {

[4 start] Side trip :)

Should this function intentionally set errno to 0 when '!err'? That way, someone doing:

virErrorSetErrnoFromLastError(); return -errno;

can return 0 for the success case when there was no last error.

Actually for the usage scenario: if (foobar() < 0) { virErrorSetErrnoFromLastError(); return -errno; } I wanted to guarantee that an errno would always be set even if foobar() forgot a virReportError() call in its error path.

[end 4]

...

+ errno = err->int1; + } else if (err && err->code == VIR_ERR_NO_MEMORY) { + errno = ENOMEM; + } else { + errno = EIO; + } +} + + +/** + * virLastErrorIsSystemErrno: + * @errnum: the errno value + * + * Check if the last error reported is a system + * error with the specific errno value + * + * Returns true if the last errr was a system error with errno == @errnum

s/errr/error/

Should we have a special case of @errnum == 0 to just state that the last error was a system error, regardless of its errno value?

I guess we could add that.

...

@@ -136,6 +136,18 @@ cleanup: }

+#define ENSURE_ERRNO(en) \ + do { \ + if (!virLastErrorIsSystemErrno(en)) { \ + virErrorPtr err = virGetLastError(); \ + fprintf(stderr, "Did not get " #en " error code: %d\n", \ + err ? err->code : 0); \

Shouldn't you also be reporting whether the error was VIR_ERR_SYSTEM_ERROR, since if it is not, then err->code is not an errno value but some other piece of data for the other error type, and might happen to match en, making the fprintf() message rather confusing?

Good poi nt.

...

fprintf(stderr, "Should not have created LXC cgroup: %d\n", -rv); goto cleanup; }

+ ENSURE_ERRNO(ENXIO);

Most of the places you used this macro, you did not have a blank line beforehand.

[end 6]

Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Instead of returning errno values, change the virCgroupKill* APIs to fully report errors.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_process.c | 15 +------ src/util/vircgroup.c | 108 ++++++++++++++++++++++++++++---------------------- 2 files changed, 62 insertions(+), 61 deletions(-)

+++ b/src/lxc/lxc_process.c @@ -769,19 +769,7 @@ int virLXCProcessStop(virLXCDriverPtr driver, }

if (priv->cgroup) { - rc = virCgroupKillPainfully(priv->cgroup); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("Failed to kill container PIDs")); - rc = -1; - goto cleanup; - } - if (rc == 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Some container PIDs refused to die"));

So we previously reported -errno or +1 for two different types of failure...

- rc = -1; - goto cleanup; - } + virCgroupKillPainfully(priv->cgroup);

...but now don't report any failure at all. Is this right? Shouldn't this be: if (virCgroupKillPainfully(priv->cgroup) < 0) goto cleanup; or even still distinguish the error message for processes that couldn't be killed (but maybe hoisting it into vircgroup.c)?

+++ b/src/util/vircgroup.c @@ -2318,9 +2318,12 @@ int virCgroupGetFreezerState(virCgroupPtr group, char **state)

#if defined HAVE_KILL && defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R +/* + * Returns 1 if some PIDs kill, 0 if non are killed, or -1 on error

s/kill/are killed/ s/non/none/

@@ -2400,15 +2408,12 @@ static void *virCgroupPidCopy(const void *name) }

/* - * Returns - * < 0 : errno that occurred - * 0 : no PIDs killed - * 1 : at least one PID killed + * Returns 1 if some PIDs kill, 0 if non are killed, or -1 on error

copy-and-paste

+ ret = virCgroupKillRecursive(group, signum); + VIR_DEBUG("Iteration %zu rc=%d", i, ret); + /* If ret == -1 we hit error, if 0 we ran out of PIDs */ + if (ret <= 0) break;

usleep(200 * 1000); } - VIR_DEBUG("Complete %d", rc); - return rc; + VIR_DEBUG("Complete %d", ret); + return ret;

Here, you can still return 1 without reporting any error, if you timed out. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

Convert the remainiing methods in vircgroup.c to report errors

s/remainiing/remaining/

instead of returning errno values.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/vircgroup.c | 559 +++++++++++++++++++++++++++++---------------------- 1 file changed, 314 insertions(+), 245 deletions(-)

@@ -645,29 +661,22 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group) for (i = 0; i < ARRAY_CARDINALITY(inherit_values); i++) { char *value;

- rc = virCgroupGetValueStr(parent, - VIR_CGROUP_CONTROLLER_CPUSET, - inherit_values[i], - &value); - if (rc != 0) { - virReportSystemError(-rc, - _("Failed to get '%s'"), inherit_values[i]); + if (virCgroupGetValueStr(parent, + VIR_CGROUP_CONTROLLER_CPUSET, + inherit_values[i], + &value) < 0)

Aha - this cleans up something I noticed in 1/3.

@@ -675,33 +684,23 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group)

- - if (rc != 0) { - virReportSystemError(-rc, - _("Failed to set '%s'"), filename); + if (virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_MEMORY, + filename, 1) < 0) return -1; - }

return 0;

Can code like this be simplified to: return virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, filename, 1);

@@ -968,11 +964,11 @@ int virCgroupRemove(virCgroupPtr group) * @group: The cgroup to add a task to * @pid: The pid of the task to add * - * Returns: 0 on success + * Returns: 0 on success, -1 on error */ int virCgroupAddTask(virCgroupPtr group, pid_t pid)

Ouch - at least src/qemu/qemu_cgroup.c calls this function, and still expects errno values: rc = virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]); if (rc < 0) { virReportSystemError(-rc, _("unable to add vcpu %zu task %d to cgroup"), i, priv->vcpupids[i]); goto cleanup; } I didn't look elsewhere; all I needed was one counterexample to state your patch is incomplete until you audit all callers impacted by semantic changes.

@@ -1019,22 +1023,28 @@ static int virCgroupAddTaskStrController(virCgroupPtr group,

+ if (virCgroupAddTaskController(group, p, controller) < 0) { + virErrorPtr err = virGetLastError(); + /* A thread that exits between when we first read the source + * tasks and now is not fatal. */ + if (err && err->code == VIR_ERR_SYSTEM_ERROR && + err->int1 == ESRCH) {

shorter as 'if (virLastErrorIsSystemErrno(ESRCH))'

@@ -1645,18 +1659,30 @@ int virCgroupSetBlkioDeviceWeight(virCgroupPtr group,

+ if (stat(path, &sb) < 0) { + virReportSystemError(errno, + _("Path '%s' is not accessible"), + path); + return -1; + }

- if (!S_ISBLK(sb.st_mode)) - return -EINVAL; + if (!S_ISBLK(sb.st_mode)) { + virReportSystemError(errno,

errno is wrong here - stat() succeeded. You want to explicitly report EINVAL.

@@ -2582,8 +2648,9 @@ static char *virCgroupIdentifyRoot(virCgroupPtr group) return NULL; }

- ignore_value(VIR_STRNDUP_QUIET(ret, group->controllers[i].mountPoint, - tmp - group->controllers[i].mountPoint)); + if (VIR_STRNDUP(ret, group->controllers[i].mountPoint, + tmp - group->controllers[i].mountPoint) < 0) + return NULL; return ret;

You can still use ignore_value() here rather than 'if', if desired, since ret will be NULL if VIR_STRNDUP fails. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, Jul 19, 2013 at 03:51:25PM +0100, Daniel P. Berrange wrote:

On Thu, Jul 18, 2013 at 09:59:47PM -0600, Eric Blake wrote:

...

On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:

...

From: "Daniel P. Berrange" <berrange@redhat.com>

Convert the remainiing methods in vircgroup.c to report errors

s/remainiing/remaining/

...

instead of returning errno values.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/vircgroup.c | 559 +++++++++++++++++++++++++++++---------------------- 1 file changed, 314 insertions(+), 245 deletions(-)

...

...

@@ -968,11 +964,11 @@ int virCgroupRemove(virCgroupPtr group) * @group: The cgroup to add a task to * @pid: The pid of the task to add * - * Returns: 0 on success + * Returns: 0 on success, -1 on error */ int virCgroupAddTask(virCgroupPtr group, pid_t pid)

Ouch - at least src/qemu/qemu_cgroup.c calls this function, and still expects errno values:

rc = virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]); if (rc < 0) { virReportSystemError(-rc, _("unable to add vcpu %zu task %d to cgroup"), i, priv->vcpupids[i]); goto cleanup; }

I didn't look elsewhere; all I needed was one counterexample to state your patch is incomplete until you audit all callers impacted by semantic changes.

Fixed those.

This time with the diff attached. diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 3cec8e2..d15fa67 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -605,13 +605,8 @@ virCgroupPtr virLXCCgroupJoin(virDomainDefPtr def) if (!(cgroup = virLXCCgroupCreate(def, true))) return NULL; - rc = virCgroupAddTask(cgroup, getpid()); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to add task %d to cgroup for domain %s"), - getpid(), def->name); + if (virCgroupAddTask(cgroup, getpid()) < 0) goto cleanup; - } ret = 0; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2df80bc..c6364fc 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -975,13 +975,8 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) goto cleanup; /* move the thread for vcpu to sub dir */ - rc = virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]); - if (rc < 0) { - virReportSystemError(-rc, - _("unable to add vcpu %zu task %d to cgroup"), - i, priv->vcpupids[i]); + if (virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]) < 0) goto cleanup; - } if (period || quota) { if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) @@ -1118,13 +1113,8 @@ qemuAddToCgroup(virDomainObjPtr vm) if (priv->cgroup == NULL) return 0; /* Not supported, so claim success */ - rc = virCgroupAddTask(priv->cgroup, getpid()); - if (rc != 0) { - virReportSystemError(-rc, - _("unable to add domain %s task %d to cgroup"), - vm->def->name, getpid()); + if (virCgroupAddTask(priv->cgroup, getpid()) < 0) return -1; - } return 0; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index bab7503..9d63edd 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1062,12 +1062,10 @@ static int virCgroupAddTaskStrController(virCgroupPtr group, virErrorPtr err = virGetLastError(); /* A thread that exits between when we first read the source * tasks and now is not fatal. */ - if (err && err->code == VIR_ERR_SYSTEM_ERROR && - err->int1 == ESRCH) { + if (virLastErrorIsSystemErrno(ESRCH)) virResetLastError(); - } else { + else goto cleanup; - } } next = strchr(cur, '\n'); @@ -1711,7 +1709,7 @@ int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, } if (!S_ISBLK(sb.st_mode)) { - virReportSystemError(errno, + virReportSystemError(EINVAL, _("Path '%s' must be a block device"), path); return -1; Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, Jul 19, 2013 at 09:51:03AM -0600, Eric Blake wrote:

On 07/19/2013 08:54 AM, Daniel P. Berrange wrote:

...

...

...

rc = virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]); if (rc < 0) { virReportSystemError(-rc, _("unable to add vcpu %zu task %d to cgroup"), i, priv->vcpupids[i]); goto cleanup; }

I didn't look elsewhere; all I needed was one counterexample to state your patch is incomplete until you audit all callers impacted by semantic changes.

Fixed those.

This time with the diff attached.

...

+++ b/src/qemu/qemu_cgroup.c

Still incomplete - you fixed virCgroupAddTask callers, but didn't audit for others. At least in this file, we have:

Sigh, you're right. I clearly forgot to change any of the callers. diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 3cec8e2..025720d 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -36,33 +36,18 @@ static int virLXCCgroupSetupCpuTune(virDomainDefPtr def, virCgroupPtr cgroup) { int ret = -1; - if (def->cputune.shares != 0) { - int rc = virCgroupSetCpuShares(cgroup, def->cputune.shares); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io cpu shares for domain %s"), - def->name); - goto cleanup; - } - } - if (def->cputune.quota != 0) { - int rc = virCgroupSetCpuCfsQuota(cgroup, def->cputune.quota); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io cpu quota for domain %s"), - def->name); - goto cleanup; - } - } - if (def->cputune.period != 0) { - int rc = virCgroupSetCpuCfsPeriod(cgroup, def->cputune.period); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io cpu period for domain %s"), - def->name); - goto cleanup; - } - } + if (def->cputune.shares != 0 && + virCgroupSetCpuShares(cgroup, def->cputune.shares) < 0) + goto cleanup; + + if (def->cputune.quota != 0 && + virCgroupSetCpuCfsQuota(cgroup, def->cputune.quota) < 0) + goto cleanup; + + if (def->cputune.period != 0 && + virCgroupSetCpuCfsPeriod(cgroup, def->cputune.period) < 0) + goto cleanup; + ret = 0; cleanup: return ret; @@ -73,7 +58,7 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, virCgroupPtr cgroup, virBitmapPtr nodemask) { - int rc = 0; + int ret = -1; char *mask = NULL; if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO && @@ -85,12 +70,8 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, return -1; } - rc = virCgroupSetCpusetCpus(cgroup, mask); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("Unable to set cpuset.cpus")); + if (virCgroupSetCpusetCpus(cgroup, mask) < 0) goto cleanup; - } } if ((def->numatune.memory.nodemask || @@ -109,14 +90,14 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, return -1; } - rc = virCgroupSetCpusetMems(cgroup, mask); - if (rc < 0) - virReportSystemError(-rc, "%s", _("Unable to set cpuset.mems")); + if (virCgroupSetCpusetMems(cgroup, mask) < 0) + goto cleanup; } + ret = 0; cleanup: VIR_FREE(mask); - return rc; + return ret; } @@ -124,31 +105,18 @@ static int virLXCCgroupSetupBlkioTune(virDomainDefPtr def, virCgroupPtr cgroup) { size_t i; - int rc; - - if (def->blkio.weight) { - rc = virCgroupSetBlkioWeight(cgroup, def->blkio.weight); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set Blkio weight for domain %s"), - def->name); - return -1; - } - } + + if (def->blkio.weight && + virCgroupSetBlkioWeight(cgroup, def->blkio.weight) < 0) + return -1; if (def->blkio.ndevices) { for (i = 0; i < def->blkio.ndevices; i++) { virBlkioDeviceWeightPtr dw = &def->blkio.devices[i]; if (!dw->weight) continue; - rc = virCgroupSetBlkioDeviceWeight(cgroup, dw->path, dw->weight); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io device weight " - "for domain %s"), - def->name); + if (virCgroupSetBlkioDeviceWeight(cgroup, dw->path, dw->weight) < 0) return -1; - } } } @@ -160,45 +128,21 @@ static int virLXCCgroupSetupMemTune(virDomainDefPtr def, virCgroupPtr cgroup) { int ret = -1; - int rc; - rc = virCgroupSetMemory(cgroup, def->mem.max_balloon); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set memory limit for domain %s"), - def->name); + if (virCgroupSetMemory(cgroup, def->mem.max_balloon) < 0) goto cleanup; - } - if (def->mem.hard_limit) { - rc = virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set memory hard limit for domain %s"), - def->name); - goto cleanup; - } - } + if (def->mem.hard_limit && + virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit) < 0) + goto cleanup; - if (def->mem.soft_limit) { - rc = virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set memory soft limit for domain %s"), - def->name); - goto cleanup; - } - } + if (def->mem.soft_limit && + virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit) < 0) + goto cleanup; - if (def->mem.swap_hard_limit) { - rc = virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set swap hard limit for domain %s"), - def->name); - goto cleanup; - } - } + if (def->mem.swap_hard_limit && + virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit) < 0) + goto cleanup; ret = 0; cleanup: @@ -306,32 +250,20 @@ cleanup: int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo) { - int ret = -1, rc; + int ret = -1; virCgroupPtr cgroup; if (virCgroupNewSelf(&cgroup) < 0) return -1; - rc = virLXCCgroupGetMemStat(cgroup, meminfo); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("Unable to get memory cgroup stat info")); + if (virLXCCgroupGetMemStat(cgroup, meminfo) < 0) goto cleanup; - } - rc = virLXCCgroupGetMemTotal(cgroup, meminfo); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("Unable to get memory cgroup total")); + if (virLXCCgroupGetMemTotal(cgroup, meminfo) < 0) goto cleanup; - } - rc = virLXCCgroupGetMemUsage(cgroup, meminfo); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("Unable to get memory cgroup stat usage")); + if (virLXCCgroupGetMemUsage(cgroup, meminfo) < 0) goto cleanup; - } virLXCCgroupGetMemSwapTotal(cgroup, meminfo); virLXCCgroupGetMemSwapUsage(cgroup, meminfo); @@ -360,17 +292,11 @@ virLXCSetupHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virCgroupPtr cgroup = opaque; - int rc; VIR_DEBUG("Process path '%s' for USB device", path); - rc = virCgroupAllowDevicePath(cgroup, path, - VIR_CGROUP_DEVICE_RW); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to allow device %s"), - path); + if (virCgroupAllowDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RW) < 0) return -1; - } return 0; } @@ -382,17 +308,11 @@ virLXCTeardownHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virCgroupPtr cgroup = opaque; - int rc; VIR_DEBUG("Process path '%s' for USB device", path); - rc = virCgroupDenyDevicePath(cgroup, path, - VIR_CGROUP_DEVICE_RW); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to deny device %s"), - path); + if (virCgroupDenyDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RW) < 0) return -1; - } return 0; } @@ -402,7 +322,6 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, virCgroupPtr cgroup) { int ret = -1; - int rc; size_t i; static virLXCCgroupDevicePolicy devices[] = { {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_NULL}, @@ -415,62 +334,42 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, {'c', LXC_DEV_MAJ_FUSE, LXC_DEV_MIN_FUSE}, {0, 0, 0}}; - rc = virCgroupDenyAllDevices(cgroup); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to deny devices for domain %s"), - def->name); + if (virCgroupDenyAllDevices(cgroup) < 0) goto cleanup; - } for (i = 0; devices[i].type != 0; i++) { virLXCCgroupDevicePolicyPtr dev = &devices[i]; - rc = virCgroupAllowDevice(cgroup, - dev->type, - dev->major, - dev->minor, - VIR_CGROUP_DEVICE_RWM); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to allow device %c:%d:%d for domain %s"), - dev->type, dev->major, dev->minor, def->name); + if (virCgroupAllowDevice(cgroup, + dev->type, + dev->major, + dev->minor, + VIR_CGROUP_DEVICE_RWM) < 0) goto cleanup; - } } for (i = 0; i < def->ndisks; i++) { if (def->disks[i]->type != VIR_DOMAIN_DISK_TYPE_BLOCK) continue; - rc = virCgroupAllowDevicePath(cgroup, - def->disks[i]->src, - (def->disks[i]->readonly ? - VIR_CGROUP_DEVICE_READ : - VIR_CGROUP_DEVICE_RW) | - VIR_CGROUP_DEVICE_MKNOD); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to allow device %s for domain %s"), - def->disks[i]->src, def->name); + if (virCgroupAllowDevicePath(cgroup, + def->disks[i]->src, + (def->disks[i]->readonly ? + VIR_CGROUP_DEVICE_READ : + VIR_CGROUP_DEVICE_RW) | + VIR_CGROUP_DEVICE_MKNOD) < 0) goto cleanup; - } } for (i = 0; i < def->nfss; i++) { if (def->fss[i]->type != VIR_DOMAIN_FS_TYPE_BLOCK) continue; - rc = virCgroupAllowDevicePath(cgroup, - def->fss[i]->src, - def->fss[i]->readonly ? - VIR_CGROUP_DEVICE_READ : - VIR_CGROUP_DEVICE_RW); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to allow device %s for domain %s"), - def->fss[i]->src, def->name); + if (virCgroupAllowDevicePath(cgroup, + def->fss[i]->src, + def->fss[i]->readonly ? + VIR_CGROUP_DEVICE_READ : + VIR_CGROUP_DEVICE_RW) < 0) goto cleanup; - } } for (i = 0; i < def->nhostdevs; i++) { @@ -520,14 +419,9 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, } } - rc = virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY, - VIR_CGROUP_DEVICE_RWM); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to allow PTY devices for domain %s"), - def->name); + if (virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY, + VIR_CGROUP_DEVICE_RWM) < 0) goto cleanup; - } ret = 0; cleanup: @@ -600,18 +494,12 @@ virCgroupPtr virLXCCgroupJoin(virDomainDefPtr def) { virCgroupPtr cgroup = NULL; int ret = -1; - int rc; if (!(cgroup = virLXCCgroupCreate(def, true))) return NULL; - rc = virCgroupAddTask(cgroup, getpid()); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to add task %d to cgroup for domain %s"), - getpid(), def->name); + if (virCgroupAddTask(cgroup, getpid()) < 0) goto cleanup; - } ret = 0; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e005d8d..0fa98df 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -562,7 +562,7 @@ static int lxcDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { virDomainObjPtr vm; - int ret = -1, rc; + int ret = -1; virLXCDomainObjPrivatePtr priv; if (!(vm = lxcDomObjFromDomain(dom))) @@ -584,14 +584,12 @@ static int lxcDomainGetInfo(virDomainPtr dom, "%s", _("Cannot read cputime for domain")); goto cleanup; } - if ((rc = virCgroupGetMemoryUsage(priv->cgroup, &(info->memory))) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Cannot read memory usage for domain")); - if (rc == -ENOENT) { - /* Don't fail if we can't read memory usage due to a lack of - * kernel support */ + if (virCgroupGetMemoryUsage(priv->cgroup, &(info->memory)) < 0) { + /* Don't fail if we can't read memory usage due to a lack of + * kernel support */ + if (virLastErrorIsSystemErrno(ENOENT)) info->memory = 0; - } else + else goto cleanup; } } @@ -746,7 +744,6 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, size_t i; virDomainObjPtr vm = NULL; int ret = -1; - int rc; virLXCDomainObjPrivatePtr priv; virCheckFlags(0, -1); @@ -773,26 +770,14 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, virTypedParameterPtr param = ¶ms[i]; if (STREQ(param->field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) { - rc = virCgroupSetMemoryHardLimit(priv->cgroup, params[i].value.ul); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to set memory hard_limit tunable")); + if (virCgroupSetMemoryHardLimit(priv->cgroup, params[i].value.ul) < 0) ret = -1; - } } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) { - rc = virCgroupSetMemorySoftLimit(priv->cgroup, params[i].value.ul); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to set memory soft_limit tunable")); + if (virCgroupSetMemorySoftLimit(priv->cgroup, params[i].value.ul) < 0) ret = -1; - } } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) { - rc = virCgroupSetMemSwapHardLimit(priv->cgroup, params[i].value.ul); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to set swap_hard_limit tunable")); + if (virCgroupSetMemSwapHardLimit(priv->cgroup, params[i].value.ul) < 0) ret = -1; - } } } @@ -812,7 +797,6 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, virDomainObjPtr vm = NULL; unsigned long long val; int ret = -1; - int rc; virLXCDomainObjPrivatePtr priv; virCheckFlags(0, -1); @@ -838,34 +822,22 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, switch (i) { case 0: /* fill memory hard limit here */ - rc = virCgroupGetMemoryHardLimit(priv->cgroup, &val); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get memory hard limit")); + if (virCgroupGetMemoryHardLimit(priv->cgroup, &val) < 0) goto cleanup; - } if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, val) < 0) goto cleanup; break; case 1: /* fill memory soft limit here */ - rc = virCgroupGetMemorySoftLimit(priv->cgroup, &val); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get memory soft limit")); + if (virCgroupGetMemorySoftLimit(priv->cgroup, &val) < 0) goto cleanup; - } if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SOFT_LIMIT, VIR_TYPED_PARAM_ULLONG, val) < 0) goto cleanup; break; case 2: /* fill swap hard limit here */ - rc = virCgroupGetMemSwapHardLimit(priv->cgroup, &val); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get swap hard limit")); + if (virCgroupGetMemSwapHardLimit(priv->cgroup, &val) < 0) goto cleanup; - } if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, val) < 0) @@ -1676,21 +1648,11 @@ static int lxcGetVcpuBWLive(virCgroupPtr cgroup, unsigned long long *period, long long *quota) { - int rc; - - rc = virCgroupGetCpuCfsPeriod(cgroup, period); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("unable to get cpu bandwidth period tunable")); + if (virCgroupGetCpuCfsPeriod(cgroup, period) < 0) return -1; - } - rc = virCgroupGetCpuCfsQuota(cgroup, quota); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("unable to get cpu bandwidth tunable")); + if (virCgroupGetCpuCfsQuota(cgroup, quota) < 0) return -1; - } return 0; } @@ -1699,7 +1661,6 @@ lxcGetVcpuBWLive(virCgroupPtr cgroup, unsigned long long *period, static int lxcSetVcpuBWLive(virCgroupPtr cgroup, unsigned long long period, long long quota) { - int rc; unsigned long long old_period; if (period == 0 && quota == 0) @@ -1707,39 +1668,23 @@ static int lxcSetVcpuBWLive(virCgroupPtr cgroup, unsigned long long period, if (period) { /* get old period, and we can rollback if set quota failed */ - rc = virCgroupGetCpuCfsPeriod(cgroup, &old_period); - if (rc < 0) { - virReportSystemError(-rc, - "%s", _("Unable to get cpu bandwidth period")); + if (virCgroupGetCpuCfsPeriod(cgroup, &old_period) < 0) return -1; - } - rc = virCgroupSetCpuCfsPeriod(cgroup, period); - if (rc < 0) { - virReportSystemError(-rc, - "%s", _("Unable to set cpu bandwidth period")); + if (virCgroupSetCpuCfsPeriod(cgroup, period) < 0) return -1; - } } if (quota) { - rc = virCgroupSetCpuCfsQuota(cgroup, quota); - if (rc < 0) { - virReportSystemError(-rc, - "%s", _("Unable to set cpu bandwidth quota")); - goto cleanup; - } + if (virCgroupSetCpuCfsQuota(cgroup, quota) < 0) + goto error; } return 0; -cleanup: - if (period) { - rc = virCgroupSetCpuCfsPeriod(cgroup, old_period); - if (rc < 0) - virReportSystemError(-rc, "%s", - _("Unable to rollback cpu bandwidth period")); - } +error: + if (period) + virCgroupSetCpuCfsPeriod(cgroup, old_period); return -1; } @@ -1808,12 +1753,8 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_CPU_SHARES)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = virCgroupSetCpuShares(priv->cgroup, params[i].value.ul); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to set cpu shares tunable")); + if (virCgroupSetCpuShares(priv->cgroup, params[i].value.ul) < 0) goto cleanup; - } vm->def->cputune.shares = params[i].value.ul; } @@ -1942,12 +1883,8 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; } - rc = virCgroupGetCpuShares(priv->cgroup, &shares); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get cpu shares tunable")); + if (virCgroupGetCpuShares(priv->cgroup, &shares) < 0) goto cleanup; - } if (*nparams > 1 && cpu_bw_status) { rc = lxcGetVcpuBWLive(priv->cgroup, &period, "a); @@ -2047,20 +1984,14 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr param = ¶ms[i]; if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - int rc; - if (params[i].value.ui > 1000 || params[i].value.ui < 100) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("out of blkio weight range.")); goto cleanup; } - rc = virCgroupSetBlkioWeight(priv->cgroup, params[i].value.ui); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to set blkio weight tunable")); + if (virCgroupSetBlkioWeight(priv->cgroup, params[i].value.ui) < 0) goto cleanup; - } } } } @@ -2110,7 +2041,6 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; unsigned int val; int ret = -1; - int rc; virLXCDomainObjPrivatePtr priv; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -2151,12 +2081,8 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, switch (i) { case 0: /* fill blkio weight here */ - rc = virCgroupGetBlkioWeight(priv->cgroup, &val); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get blkio weight")); + if (virCgroupGetBlkioWeight(priv->cgroup, &val) < 0) goto cleanup; - } if (virTypedParameterAssign(param, VIR_DOMAIN_BLKIO_WEIGHT, VIR_TYPED_PARAM_UINT, val) < 0) goto cleanup; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2df80bc..eed396f 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -54,25 +54,23 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, { virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; + int ret; VIR_DEBUG("Process path %s for disk", path); - rc = virCgroupAllowDevicePath(priv->cgroup, path, + ret = virCgroupAllowDevicePath(priv->cgroup, path, (disk->readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW)); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, - disk->readonly ? "r" : "rw", rc); - if (rc < 0) { - if (rc == -EACCES) { /* Get this for root squash NFS */ - VIR_DEBUG("Ignoring EACCES for %s", path); - } else { - virReportSystemError(-rc, - _("Unable to allow access for disk path %s"), - path); - return -1; - } + disk->readonly ? "r" : "rw", ret == 0); + + /* Get this for root squash NFS */ + if (ret < 0 && + virLastErrorIsSystemErrno(EACCES)) { + VIR_DEBUG("Ignoring EACCES for %s", path); + virResetLastError(); + ret = 0; } - return 0; + return ret; } @@ -98,23 +96,21 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, { virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; + int ret; VIR_DEBUG("Process path %s for disk", path); - rc = virCgroupDenyDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RWM); - virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rc); - if (rc < 0) { - if (rc == -EACCES) { /* Get this for root squash NFS */ - VIR_DEBUG("Ignoring EACCES for %s", path); - } else { - virReportSystemError(-rc, - _("Unable to deny access for disk path %s"), - path); - return -1; - } + ret = virCgroupDenyDevicePath(priv->cgroup, path, + VIR_CGROUP_DEVICE_RWM); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", ret == 0); + + /* Get this for root squash NFS */ + if (ret < 0 && + virLastErrorIsSystemErrno(EACCES)) { + VIR_DEBUG("Ignoring EACCES for %s", path); + virResetLastError(); + ret = 0; } - return 0; + return ret; } @@ -135,31 +131,25 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm, } static int -qemuSetupChrSourceCgroup(virDomainDefPtr def, +qemuSetupChrSourceCgroup(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainChrSourceDefPtr dev, void *opaque) { virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; + int ret; if (dev->type != VIR_DOMAIN_CHR_TYPE_DEV) return 0; VIR_DEBUG("Process path '%s' for device", dev->data.file.path); - rc = virCgroupAllowDevicePath(priv->cgroup, dev->data.file.path, - VIR_CGROUP_DEVICE_RW); + ret = virCgroupAllowDevicePath(priv->cgroup, dev->data.file.path, + VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", - dev->data.file.path, "rw", rc); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to allow device %s for %s"), - dev->data.file.path, def->name); - return -1; - } + dev->data.file.path, "rw", ret == 0); - return 0; + return ret; } static int @@ -176,18 +166,18 @@ qemuSetupTPMCgroup(virDomainDefPtr def, virDomainTPMDefPtr dev, void *opaque) { - int rc = 0; + int ret = 0; switch (dev->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - rc = qemuSetupChrSourceCgroup(def, &dev->data.passthrough.source, - opaque); + ret = qemuSetupChrSourceCgroup(def, &dev->data.passthrough.source, + opaque); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; } - return rc; + return ret; } @@ -198,20 +188,14 @@ qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, { virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; + int ret; VIR_DEBUG("Process path '%s' for USB device", path); - rc = virCgroupAllowDevicePath(priv->cgroup, path, + ret = virCgroupAllowDevicePath(priv->cgroup, path, VIR_CGROUP_DEVICE_RW); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rc); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to allow device %s"), - path); - return -1; - } + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", ret == 0); - return 0; + return ret; } static int @@ -221,25 +205,19 @@ qemuSetupHostScsiDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, { virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; + int ret; VIR_DEBUG("Process path '%s' for SCSI device", path); - rc = virCgroupAllowDevicePath(priv->cgroup, path, - virSCSIDeviceGetReadonly(dev) ? - VIR_CGROUP_DEVICE_READ : - VIR_CGROUP_DEVICE_RW); + ret = virCgroupAllowDevicePath(priv->cgroup, path, + virSCSIDeviceGetReadonly(dev) ? + VIR_CGROUP_DEVICE_READ : + VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, - virSCSIDeviceGetReadonly(dev) ? "r" : "rw", rc); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to allow device %s"), - path); - return -1; - } + virSCSIDeviceGetReadonly(dev) ? "r" : "rw", ret == 0); - return 0; + return ret; } int @@ -267,7 +245,7 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (dev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { - int rc; + int rv; pci = virPCIDeviceNew(dev->source.subsys.u.pci.addr.domain, dev->source.subsys.u.pci.addr.bus, @@ -280,17 +258,12 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, goto cleanup; VIR_DEBUG("Cgroup allow %s for PCI device assignment", path); - rc = virCgroupAllowDevicePath(priv->cgroup, path, + rv = virCgroupAllowDevicePath(priv->cgroup, path, VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupPath(vm, priv->cgroup, - "allow", path, "rw", rc); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to allow access " - "for device path %s"), - path); + "allow", path, "rw", rv == 0); + if (rv < 0) goto cleanup; - } } break; @@ -366,7 +339,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (dev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { - int rc; + int rv; pci = virPCIDeviceNew(dev->source.subsys.u.pci.addr.domain, dev->source.subsys.u.pci.addr.bus, @@ -379,17 +352,12 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, goto cleanup; VIR_DEBUG("Cgroup deny %s for PCI device assignment", path); - rc = virCgroupDenyDevicePath(priv->cgroup, path, + rv = virCgroupDenyDevicePath(priv->cgroup, path, VIR_CGROUP_DEVICE_RWM); virDomainAuditCgroupPath(vm, priv->cgroup, - "deny", path, "rwm", rc); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to deny access " - "for device path %s"), - path); + "deny", path, "rwm", rv == 0); + if (rv < 0) goto cleanup; - } } break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: @@ -411,7 +379,6 @@ static int qemuSetupBlkioCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - int rc = -1; size_t i; if (!virCgroupHasController(priv->cgroup, @@ -425,30 +392,18 @@ qemuSetupBlkioCgroup(virDomainObjPtr vm) } } - if (vm->def->blkio.weight != 0) { - rc = virCgroupSetBlkioWeight(priv->cgroup, vm->def->blkio.weight); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io weight for domain %s"), - vm->def->name); - return -1; - } - } + if (vm->def->blkio.weight != 0 && + virCgroupSetBlkioWeight(priv->cgroup, vm->def->blkio.weight) < 0) + return -1; if (vm->def->blkio.ndevices) { for (i = 0; i < vm->def->blkio.ndevices; i++) { virBlkioDeviceWeightPtr dw = &vm->def->blkio.devices[i]; if (!dw->weight) continue; - rc = virCgroupSetBlkioDeviceWeight(priv->cgroup, dw->path, - dw->weight); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io device weight " - "for domain %s"), - vm->def->name); + if (virCgroupSetBlkioDeviceWeight(priv->cgroup, dw->path, + dw->weight) < 0) return -1; - } } } @@ -460,7 +415,6 @@ static int qemuSetupMemoryCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; if (!virCgroupHasController(priv->cgroup,VIR_CGROUP_CONTROLLER_MEMORY)) { if (vm->def->mem.hard_limit != 0 || @@ -474,33 +428,17 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm) } } - rc = virCgroupSetMemoryHardLimit(priv->cgroup, - qemuDomainMemoryLimit(vm->def)); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set memory hard limit for domain %s"), - vm->def->name); + if (virCgroupSetMemoryHardLimit(priv->cgroup, + qemuDomainMemoryLimit(vm->def)) < 0) return -1; - } - if (vm->def->mem.soft_limit != 0) { - rc = virCgroupSetMemorySoftLimit(priv->cgroup, vm->def->mem.soft_limit); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set memory soft limit for domain %s"), - vm->def->name); - return -1; - } - } - if (vm->def->mem.swap_hard_limit != 0) { - rc = virCgroupSetMemSwapHardLimit(priv->cgroup, vm->def->mem.swap_hard_limit); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set swap hard limit for domain %s"), - vm->def->name); - return -1; - } - } + if (vm->def->mem.soft_limit != 0 && + virCgroupSetMemorySoftLimit(priv->cgroup, vm->def->mem.soft_limit) < 0) + return -1; + + if (vm->def->mem.swap_hard_limit != 0 && + virCgroupSetMemSwapHardLimit(priv->cgroup, vm->def->mem.swap_hard_limit) < 0) + return -1; return 0; } @@ -513,23 +451,22 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = NULL; const char *const *deviceACL = NULL; - int rc = -1; + int rv = -1; int ret = -1; size_t i; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; - rc = virCgroupDenyAllDevices(priv->cgroup); - virDomainAuditCgroup(vm, priv->cgroup, "deny", "all", rc == 0); - if (rc != 0) { - if (rc == -EPERM) { + rv = virCgroupDenyAllDevices(priv->cgroup); + virDomainAuditCgroup(vm, priv->cgroup, "deny", "all", rv == 0); + if (rv < 0) { + if (virLastErrorIsSystemErrno(EPERM)) { + virResetLastError(); VIR_WARN("Group devices ACL is not accessible, disabling whitelisting"); return 0; } - virReportSystemError(-rc, - _("Unable to deny all devices for %s"), vm->def->name); goto cleanup; } @@ -538,15 +475,12 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; } - rc = virCgroupAllowDeviceMajor(priv->cgroup, 'c', DEVICE_PTY_MAJOR, + rv = virCgroupAllowDeviceMajor(priv->cgroup, 'c', DEVICE_PTY_MAJOR, VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupMajor(vm, priv->cgroup, "allow", DEVICE_PTY_MAJOR, - "pty", "rw", rc == 0); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to allow /dev/pts/ devices")); + "pty", "rw", rv == 0); + if (rv < 0) goto cleanup; - } cfg = virQEMUDriverGetConfig(driver); deviceACL = cfg->cgroupDeviceACL ? @@ -558,15 +492,12 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, ((vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && cfg->vncAllowHostAudio) || (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL)))) { - rc = virCgroupAllowDeviceMajor(priv->cgroup, 'c', DEVICE_SND_MAJOR, + rv = virCgroupAllowDeviceMajor(priv->cgroup, 'c', DEVICE_SND_MAJOR, VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupMajor(vm, priv->cgroup, "allow", DEVICE_SND_MAJOR, - "sound", "rw", rc == 0); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to allow /dev/snd/ devices")); + "sound", "rw", rv == 0); + if (rv < 0) goto cleanup; - } } for (i = 0; deviceACL[i] != NULL; i++) { @@ -576,16 +507,12 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, continue; } - rc = virCgroupAllowDevicePath(priv->cgroup, deviceACL[i], + rv = virCgroupAllowDevicePath(priv->cgroup, deviceACL[i], VIR_CGROUP_DEVICE_RW); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", deviceACL[i], "rw", rc); - if (rc < 0 && - rc != -ENOENT) { - virReportSystemError(-rc, - _("unable to allow device %s"), - deviceACL[i]); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", deviceACL[i], "rw", rv == 0); + if (rv < 0 && + !virLastErrorIsSystemErrno(ENOENT)) goto cleanup; - } } if (virDomainChrDefForeach(vm->def, @@ -620,7 +547,6 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; char *mem_mask = NULL; char *cpu_mask = NULL; - int rc; int ret = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) @@ -643,14 +569,8 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm, goto cleanup; } - rc = virCgroupSetCpusetMems(priv->cgroup, mem_mask); - - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set cpuset.mems for domain %s"), - vm->def->name); + if (virCgroupSetCpusetMems(priv->cgroup, mem_mask) < 0) goto cleanup; - } } if (vm->def->cpumask || @@ -672,12 +592,8 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm, goto cleanup; } - if ((rc = virCgroupSetCpusetCpus(priv->cgroup, cpu_mask)) != 0) { - virReportSystemError(-rc, - _("Unable to set cpuset.cpus for domain %s"), - vm->def->name); + if (virCgroupSetCpusetCpus(priv->cgroup, cpu_mask) < 0) goto cleanup; - } } ret = 0; @@ -692,7 +608,6 @@ static int qemuSetupCpuCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - int rc = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { if (vm->def->cputune.shares) { @@ -704,15 +619,9 @@ qemuSetupCpuCgroup(virDomainObjPtr vm) } } - if (vm->def->cputune.shares) { - rc = virCgroupSetCpuShares(priv->cgroup, vm->def->cputune.shares); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io cpu shares for domain %s"), - vm->def->name); - return -1; - } - } + if (vm->def->cputune.shares && + virCgroupSetCpuShares(priv->cgroup, vm->def->cputune.shares) < 0) + return -1; return 0; } @@ -723,7 +632,7 @@ qemuInitCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm, bool startup) { - int rc = -1; + int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; virCgroupPtr parent = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -795,11 +704,11 @@ qemuInitCgroup(virQEMUDriverPtr driver, } done: - rc = 0; + ret = 0; cleanup: virCgroupFree(&parent); virObjectUnref(cfg); - return rc; + return ret; } @@ -847,7 +756,6 @@ qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, unsigned long long period, long long quota) { - int rc; unsigned long long old_period; if (period == 0 && quota == 0) @@ -855,39 +763,22 @@ qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, if (period) { /* get old period, and we can rollback if set quota failed */ - rc = virCgroupGetCpuCfsPeriod(cgroup, &old_period); - if (rc < 0) { - virReportSystemError(-rc, - "%s", _("Unable to get cpu bandwidth period")); + if (virCgroupGetCpuCfsPeriod(cgroup, &old_period) < 0) return -1; - } - rc = virCgroupSetCpuCfsPeriod(cgroup, period); - if (rc < 0) { - virReportSystemError(-rc, - "%s", _("Unable to set cpu bandwidth period")); + if (virCgroupSetCpuCfsPeriod(cgroup, period) < 0) return -1; - } } - if (quota) { - rc = virCgroupSetCpuCfsQuota(cgroup, quota); - if (rc < 0) { - virReportSystemError(-rc, - "%s", _("Unable to set cpu bandwidth quota")); - goto cleanup; - } - } + if (quota && + virCgroupSetCpuCfsQuota(cgroup, quota) < 0) + goto error; return 0; -cleanup: - if (period) { - rc = virCgroupSetCpuCfsPeriod(cgroup, old_period); - if (rc < 0) - virReportSystemError(-rc, "%s", - _("Unable to rollback cpu bandwidth period")); - } +error: + if (period) + ignore_value(virCgroupSetCpuCfsPeriod(cgroup, old_period)); return -1; } @@ -913,28 +804,23 @@ int qemuSetupCgroupEmulatorPin(virCgroupPtr cgroup, virBitmapPtr cpumask) { - int rc = 0; + int ret = -1; char *new_cpus = NULL; new_cpus = virBitmapFormat(cpumask); if (!new_cpus) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to convert cpu mask")); - rc = -1; goto cleanup; } - rc = virCgroupSetCpusetCpus(cgroup, new_cpus); - if (rc < 0) { - virReportSystemError(-rc, - "%s", - _("Unable to set cpuset.cpus")); + if (virCgroupSetCpusetCpus(cgroup, new_cpus) < 0) goto cleanup; - } + ret = 0; cleanup: VIR_FREE(new_cpus); - return rc; + return ret; } int @@ -943,7 +829,6 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupPtr cgroup_vcpu = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - int rc; size_t i, j; unsigned long long period = vm->def->cputune.period; long long quota = vm->def->cputune.quota; @@ -975,13 +860,8 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) goto cleanup; /* move the thread for vcpu to sub dir */ - rc = virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]); - if (rc < 0) { - virReportSystemError(-rc, - _("unable to add vcpu %zu task %d to cgroup"), - i, priv->vcpupids[i]); + if (virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]) < 0) goto cleanup; - } if (period || quota) { if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) @@ -1032,7 +912,6 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; unsigned long long period = vm->def->cputune.emulator_period; long long quota = vm->def->cputune.emulator_quota; - int rc = -1; if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { @@ -1047,14 +926,8 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, if (virCgroupNewEmulator(priv->cgroup, true, &cgroup_emulator) < 0) goto cleanup; - rc = virCgroupMoveTask(priv->cgroup, cgroup_emulator); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to move tasks from domain cgroup to " - "emulator cgroup for %s"), - vm->def->name); + if (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 0) goto cleanup; - } if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { if (!(cpumap = qemuPrepareCpumap(driver, nodemask))) @@ -1067,20 +940,16 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, } if (cpumask) { - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - rc = qemuSetupCgroupEmulatorPin(cgroup_emulator, cpumask); - if (rc < 0) - goto cleanup; - } - cpumask = NULL; /* sanity */ + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET) && + qemuSetupCgroupEmulatorPin(cgroup_emulator, cpumask) < 0) + goto cleanup; } if (period || quota) { - if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { - if ((rc = qemuSetupCgroupVcpuBW(cgroup_emulator, period, - quota)) < 0) - goto cleanup; - } + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + qemuSetupCgroupVcpuBW(cgroup_emulator, period, + quota) < 0) + goto cleanup; } virCgroupFree(&cgroup_emulator); @@ -1095,7 +964,7 @@ cleanup: virCgroupFree(&cgroup_emulator); } - return rc; + return -1; } int @@ -1113,18 +982,12 @@ int qemuAddToCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; if (priv->cgroup == NULL) return 0; /* Not supported, so claim success */ - rc = virCgroupAddTask(priv->cgroup, getpid()); - if (rc != 0) { - virReportSystemError(-rc, - _("unable to add domain %s task %d to cgroup"), - vm->def->name, getpid()); + if (virCgroupAddTask(priv->cgroup, getpid()) < 0) return -1; - } return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e88267c..9bbc5d6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7749,7 +7749,6 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < nparams; i++) { - int rc; virTypedParameterPtr param = ¶ms[i]; if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { @@ -7760,12 +7759,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, continue; } - rc = virCgroupSetBlkioWeight(priv->cgroup, params[i].value.ui); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to set blkio weight tunable")); + if (virCgroupSetBlkioWeight(priv->cgroup, params[i].value.ui) < 0) ret = -1; - } } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { size_t ndevices; virBlkioDeviceWeightPtr devices = NULL; @@ -7778,14 +7773,10 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, continue; } for (j = 0; j < ndevices; j++) { - rc = virCgroupSetBlkioDeviceWeight(priv->cgroup, - devices[j].path, - devices[j].weight); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to set io device weight " - "for path %s"), - devices[j].path); + if (virCgroupSetBlkioDeviceWeight(priv->cgroup, + devices[j].path, + devices[j].weight) < 0) { + ret = -1; break; } } @@ -7860,7 +7851,6 @@ qemuDomainGetBlkioParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; unsigned int val; int ret = -1; - int rc; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; @@ -7910,12 +7900,8 @@ qemuDomainGetBlkioParameters(virDomainPtr dom, switch (i) { case 0: /* fill blkio weight here */ - rc = virCgroupGetBlkioWeight(priv->cgroup, &val); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get blkio weight")); + if (virCgroupGetBlkioWeight(priv->cgroup, &val) < 0) goto cleanup; - } if (virTypedParameterAssign(param, VIR_DOMAIN_BLKIO_WEIGHT, VIR_TYPED_PARAM_UINT, val) < 0) goto cleanup; @@ -8040,8 +8026,8 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, bool set_hard_limit = false; bool set_soft_limit = false; virQEMUDriverConfigPtr cfg = NULL; - int ret = -1; int rc; + int ret = -1; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; @@ -8173,7 +8159,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; int ret = -1; - int rc; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; @@ -8257,12 +8242,8 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, switch (i) { case 0: /* fill memory hard limit here */ - rc = virCgroupGetMemoryHardLimit(priv->cgroup, &val); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get memory hard limit")); + if (virCgroupGetMemoryHardLimit(priv->cgroup, &val) < 0) goto cleanup; - } if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, val) < 0) @@ -8270,12 +8251,8 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, break; case 1: /* fill memory soft limit here */ - rc = virCgroupGetMemorySoftLimit(priv->cgroup, &val); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get memory soft limit")); + if (virCgroupGetMemorySoftLimit(priv->cgroup, &val) < 0) goto cleanup; - } if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SOFT_LIMIT, VIR_TYPED_PARAM_ULLONG, val) < 0) @@ -8283,13 +8260,9 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, break; case 2: /* fill swap hard limit here */ - rc = virCgroupGetMemSwapHardLimit(priv->cgroup, &val); - if (rc != 0) { - if (rc != -ENOENT) { - virReportSystemError(-rc, "%s", - _("unable to get swap hard limit")); + if (virCgroupGetMemSwapHardLimit(priv->cgroup, &val) < 0) { + if (!virLastErrorIsSystemErrno(ENOENT)) goto cleanup; - } val = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; } if (virTypedParameterAssign(param, @@ -8382,7 +8355,6 @@ qemuDomainSetNumaParameters(virDomainPtr dom, persistentDef->numatune.memory.mode = params[i].value.i; } } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { - int rc; virBitmapPtr nodeset = NULL; char *nodeset_str = NULL; @@ -8415,9 +8387,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, continue; } - if ((rc = virCgroupSetCpusetMems(priv->cgroup, nodeset_str)) != 0) { - virReportSystemError(-rc, "%s", - _("unable to set numa tunable")); + if (virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0) { virBitmapFree(nodeset); VIR_FREE(nodeset_str); ret = -1; @@ -8474,7 +8444,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; char *nodeset = NULL; int ret = -1; - int rc; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; @@ -8536,12 +8505,8 @@ qemuDomainGetNumaParameters(virDomainPtr dom, if (!nodeset && VIR_STRDUP(nodeset, "") < 0) goto cleanup; } else { - rc = virCgroupGetCpusetMems(priv->cgroup, &nodeset); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get numa nodeset")); + if (virCgroupGetCpusetMems(priv->cgroup, &nodeset) < 0) goto cleanup; - } } if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET, VIR_TYPED_PARAM_STRING, nodeset) < 0) @@ -8712,11 +8677,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_CPU_SHARES)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if ((rc = virCgroupSetCpuShares(priv->cgroup, value_ul))) { - virReportSystemError(-rc, "%s", - _("unable to set cpu shares tunable")); + if (virCgroupSetCpuShares(priv->cgroup, value_ul) < 0) goto cleanup; - } vm->def->cputune.shares = value_ul; } @@ -8823,21 +8785,11 @@ static int qemuGetVcpuBWLive(virCgroupPtr cgroup, unsigned long long *period, long long *quota) { - int rc; - - rc = virCgroupGetCpuCfsPeriod(cgroup, period); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("unable to get cpu bandwidth period tunable")); + if (virCgroupGetCpuCfsPeriod(cgroup, period) < 0) return -1; - } - rc = virCgroupGetCpuCfsQuota(cgroup, quota); - if (rc < 0) { - virReportSystemError(-rc, "%s", - _("unable to get cpu bandwidth tunable")); + if (virCgroupGetCpuCfsQuota(cgroup, quota) < 0) return -1; - } return 0; } @@ -8979,12 +8931,8 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; } - rc = virCgroupGetCpuShares(priv->cgroup, &shares); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get cpu shares tunable")); + if (virCgroupGetCpuShares(priv->cgroup, &shares) < 0) goto cleanup; - } if (*nparams > 1 && cpu_bw_status) { rc = qemuGetVcpusBWLive(vm, &period, "a); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d285aa1..3dedfe8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4522,8 +4522,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, enum qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - int ret = -1; int rc; + int ret = -1; bool restoreLabel = false; virCommandPtr cmd = NULL; int pipeFD[2] = { -1, -1 }; @@ -4557,15 +4557,12 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, * that botches pclose. */ if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { - rc = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rc); - if (rc == 1) { + int rv = virCgroupAllowDevicePath(priv->cgroup, path, + VIR_CGROUP_DEVICE_RW); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rv == 0); + if (rv == 1) { /* path was not a device, no further need for cgroup */ - } else if (rc < 0) { - virReportSystemError(-rc, - _("Unable to allow device %s for %s"), - path, vm->def->name); + } else if (rv < 0) { goto cleanup; } } @@ -4664,12 +4661,9 @@ cleanup: if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { - rc = virCgroupDenyDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RWM); - virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rc); - if (rc < 0) - VIR_WARN("Unable to deny device %s for %s %d", - path, vm->def->name, rc); + int rv = virCgroupDenyDevicePath(priv->cgroup, path, + VIR_CGROUP_DEVICE_RWM); + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rv == 0); } return ret; } Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|