On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Instead of returning raw errno values, report full libvirt
> errors in virCgroupNew* functions.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> 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?
'git format-patch -O/path/to/file' can be used to order your patch in a
more logical manner (.h first, then changes to virerror and vircgroup
prior to changes to clients). But for now, I'm just jumping around in
the email for my comments and will leave your original order intact;
hopefully the result isn't too confusing when you read my reply in
linear order rather than logical order.
> +++ 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?
> +++ 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.
> +++ b/src/qemu/qemu_cgroup.c
> @@ -727,64 +727,52 @@ int qemuInitCgroup(virQEMUDriverPtr driver,
> }
> /* We only auto-create the default partition. In other
> * cases we expec the sysadmin/app to have done so */
> - rc = virCgroupNewPartition(vm->def->resource->partition,
> - STREQ(vm->def->resource->partition, "/machine"),
> - cfg->cgroupControllers,
> - &parent);
> - if (rc != 0) {
> - if (rc == -ENXIO ||
> - rc == -EPERM ||
> - rc == -EACCES) { /* No cgroups mounts == success */
> + 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?
> 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?
> + 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?
[end 5]
> 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.
> @@ -677,8 +683,9 @@ static int virCgroupSetMemoryUseHierarchy(virCgroupPtr group)
> VIR_CGROUP_CONTROLLER_MEMORY,
> filename, &value);
> if (rc != 0) {
> - VIR_ERROR(_("Failed to read %s/%s (%d)"), group->path, filename, rc);
> - return rc;
> + virReportSystemError(-rc,
> + _("Failed to get '%s'"), filename);
Another instance - I guess there will be quite a few of them.
> @@ -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?
> @@ -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()?
> 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?
> + *
> + * 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.
[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?
> + */
> +bool virLastErrorIsSystemErrno(int errnum)
> +{
> + virErrorPtr err = virGetLastError();
> + if (!err)
> + return false;
> + if (err->code != VIR_ERR_SYSTEM_ERROR)
> + return false;
> + if (err->int1 != errnum)
> + return false;
> + return true;
> +}
> diff --git a/src/util/virerror.h b/src/util/virerror.h
> index 332a5eb..b4f2f04 100644
> --- a/src/util/virerror.h
> +++ b/src/util/virerror.h
> @@ -164,4 +164,8 @@ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
> typedef int (*virErrorLogPriorityFunc)(virErrorPtr, int);
> void virSetErrorLogPriorityFunc(virErrorLogPriorityFunc func);
>
> +void virErrorSetErrnoFromLastError(void);
> +
> +bool virLastErrorIsSystemErrno(int errnum);
> +
> #endif
[end 1]
> diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
> index 22b40b4..21529a0 100644
> --- a/tests/vircgrouptest.c
> +++ b/tests/vircgrouptest.c
[6 start]
> @@ -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?
> 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]
Overall, looks like a decent improvement. I pointed out a few things
worth cleaning up, and the idea of splitting into two patches may have
merit. It may be faster to post the changes you will squash in than a
full-blown v2, considering the size of this patch, and the most of the
conversions made sense.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list