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

[Daniel P. Berrange]

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 at redhat.com>
> > 
> > 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 at redhat.com>
> > ---
> >  src/util/vircgroup.c | 559 +++++++++++++++++++++++++++++----------------------
> >  1 file changed, 314 insertions(+), 245 deletions(-)
> > 
> 
> > @@ -645,29 +661,22 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group)
> >      for (i = 0; i < ARRAY_CARDINALITY(inherit_values); i++) {
> >          char *value;
> >  
> > -        rc = virCgroupGetValueStr(parent,
> > -                                  VIR_CGROUP_CONTROLLER_CPUSET,
> > -                                  inherit_values[i],
> > -                                  &value);
> > -        if (rc != 0) {
> > -            virReportSystemError(-rc,
> > -                                 _("Failed to get '%s'"), inherit_values[i]);
> > +        if (virCgroupGetValueStr(parent,
> > +                                 VIR_CGROUP_CONTROLLER_CPUSET,
> > +                                 inherit_values[i],
> > +                                 &value) < 0)
> 
> Aha - this cleans up something I noticed in 1/3.
> 
> > @@ -675,33 +684,23 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group)
> >  
> 
> > -
> > -    if (rc != 0) {
> > -        virReportSystemError(-rc,
> > -                             _("Failed to set '%s'"), filename);
> > +    if (virCgroupSetValueU64(group,
> > +                             VIR_CGROUP_CONTROLLER_MEMORY,
> > +                             filename, 1) < 0)
> >          return -1;
> > -    }
> >  
> >      return 0;
> 
> Can code like this be simplified to:
> 
> return virCgroupSetValueU64(group,
>                             VIR_CGROUP_CONTROLLER_MEMORY,
>                             filename, 1);

I'm not a fan of doing that, since it means it'll be re-arranged
again if we ever put move code in this method.

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

> > @@ -1645,18 +1659,30 @@ int virCgroupSetBlkioDeviceWeight(virCgroupPtr group,
> 
> > +    if (stat(path, &sb) < 0) {
> > +        virReportSystemError(errno,
> > +                             _("Path '%s' is not accessible"),
> > +                             path);
> > +        return -1;
> > +    }
> >  
> > -    if (!S_ISBLK(sb.st_mode))
> > -        return -EINVAL;
> > +    if (!S_ISBLK(sb.st_mode)) {
> > +        virReportSystemError(errno,
> 
> errno is wrong here - stat() succeeded.  You want to explicitly report
> EINVAL.
> 
> > @@ -2582,8 +2648,9 @@ static char *virCgroupIdentifyRoot(virCgroupPtr group)
> >              return NULL;
> >          }
> >  
> > -        ignore_value(VIR_STRNDUP_QUIET(ret, group->controllers[i].mountPoint,
> > -                                       tmp - group->controllers[i].mountPoint));
> > +        if (VIR_STRNDUP(ret, group->controllers[i].mountPoint,
> > +                        tmp - group->controllers[i].mountPoint) < 0)
> > +            return NULL;
> >          return ret;
> 
> You can still use ignore_value() here rather than 'if', if desired,
> since ret will be NULL if VIR_STRNDUP fails.

I don't like that kind of use of ignore_value(). It makes it really
easy for someone else to screw things up later by adding in more
code between the VIR_STRNDUP & the following 'return ret'. An
explicit 'return NULL' is safe in that scenario.


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