8:23 a.m.
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(a)redhat.com>
>
> Instead of returning raw errno values, report full libvirt
> errors in virCgroupNew* functions.
>
> Signed-off-by: Daniel P. Berrange <berrange(a)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 :|