Re: [PATCH 3/3] Convert remainder of cgroups code to report errors

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list