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