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@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?
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list