[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] Report full errors from virCgroupNew*



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.

[snip]

> 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.

Incremental diff is:

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a2b61c7..45e7f63 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1156,6 +1156,7 @@ virCgroupAddTaskController;
 virCgroupAllowDevice;
 virCgroupAllowDeviceMajor;
 virCgroupAllowDevicePath;
+virCgroupAvailable;
 virCgroupControllerTypeFromString;
 virCgroupControllerTypeToString;
 virCgroupDenyAllDevices;
@@ -1188,6 +1189,7 @@ virCgroupNewDomainDriver;
 virCgroupNewDomainPartition;
 virCgroupNewDriver;
 virCgroupNewEmulator;
+virCgroupNewIgnoreError;
 virCgroupNewPartition;
 virCgroupNewSelf;
 virCgroupNewVcpu;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index d20dcc2..2df80bc 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -731,6 +731,9 @@ qemuInitCgroup(virQEMUDriverPtr driver,
     if (!cfg->privileged)
         goto done;
 
+    if (!virCgroupAvailable())
+        goto done;
+
     virCgroupFree(&priv->cgroup);
 
     if (!vm->def->resource && startup) {
@@ -761,15 +764,8 @@ qemuInitCgroup(virQEMUDriverPtr driver,
                                   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 */
-                VIR_DEBUG("No cgroups present/configured/accessible, ignoring error");
-                virResetLastError();
+            if (virCgroupNewIgnoreError())
                 goto done;
-            }
 
             goto cleanup;
         }
@@ -785,15 +781,8 @@ qemuInitCgroup(virQEMUDriverPtr driver,
                                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();
+            if (virCgroupNewIgnoreError())
                 goto done;
-            }
 
             goto cleanup;
         }
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 12ace2e..3569046 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -67,6 +67,30 @@ typedef enum {
                                        */
 } virCgroupFlags;
 
+bool virCgroupAvailable(void)
+{
+    FILE *mounts = NULL;
+    struct mntent entry;
+    bool ret = false;
+    char buf[CGROUP_MAX_VAL];
+
+    if (!virFileExists("/proc/cgroups"))
+        return false;
+
+    if (!(mounts = fopen("/proc/mounts", "r")))
+        return false;
+
+    while (getmntent_r(mounts, &entry, buf, sizeof(buf)) != NULL) {
+        if (STREQ(entry.mnt_type, "cgroup")) {
+            ret = true;
+            break;
+        }
+    }
+
+    VIR_FORCE_FCLOSE(mounts);
+    return ret;
+}
+
 /**
  * virCgroupFree:
  *
@@ -1585,6 +1609,19 @@ int virCgroupNewEmulator(virCgroupPtr domain ATTRIBUTE_UNUSED,
 }
 
 #endif
+
+bool virCgroupNewIgnoreError(void)
+{
+    if (virLastErrorIsSystemErrno(ENXIO) ||
+        virLastErrorIsSystemErrno(EPERM) ||
+        virLastErrorIsSystemErrno(EACCES)) {
+        virResetLastError();
+        VIR_DEBUG("No cgroups present/configured/accessible, ignoring error");
+        return true;
+    }
+    return false;
+}
+
 /**
  * virCgroupSetBlkioWeight:
  *
@@ -2464,7 +2501,8 @@ static int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHas
         VIR_DEBUG("Process subdir %s", ent->d_name);
 
         if (virCgroupNew(ent->d_name, group, -1, &subgroup) < 0) {
-            rc = -EIO;
+            virErrorSetErrnoFromLastError();
+            rc = -errno;
             goto cleanup;
         }
 
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index b030e4a..0ec8b67 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -46,6 +46,8 @@ enum {
 
 VIR_ENUM_DECL(virCgroupController);
 
+bool virCgroupAvailable(void);
+
 int virCgroupNewPartition(const char *path,
                           bool create,
                           int controllers,
@@ -84,6 +86,8 @@ int virCgroupNewEmulator(virCgroupPtr domain,
                          virCgroupPtr *group)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
 
+bool virCgroupNewIgnoreError(void);
+
 int virCgroupPathOfController(virCgroupPtr group,
                               int controller,
                               const char *key,
diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
index 21529a0..a996898 100644
--- a/tests/vircgrouptest.c
+++ b/tests/vircgrouptest.c
@@ -140,8 +140,8 @@ cleanup:
     do {                                                            \
     if (!virLastErrorIsSystemErrno(en)) {                           \
         virErrorPtr err = virGetLastError();                        \
-        fprintf(stderr, "Did not get " #en " error code: %d\n",     \
-                err ? err->code : 0);                               \
+        fprintf(stderr, "Did not get " #en " error code: %d:%d\n",  \
+                err ? err->code : 0, err ? err->int1 : 0);          \
         goto cleanup;                                               \
     } } while (0)
 
@@ -193,7 +193,6 @@ static int testCgroupNewForDriver(const void *args ATTRIBUTE_UNUSED)
         fprintf(stderr, "Should not have created LXC cgroup: %d\n", -rv);
         goto cleanup;
     }
-
     ENSURE_ERRNO(ENXIO);
 
     /* Asking for small combination since devices is not mounted */


-- 
|: 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]