On Thu, Jul 18, 2013 at 09:36:59PM -0600, Eric Blake wrote:
> On 07/18/2013 09:00 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > Instead of returning errno values, change the virCgroupKill*
> > APIs to fully report errors.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> > ---
> > src/lxc/lxc_process.c | 15 +------
> > src/util/vircgroup.c | 108 ++++++++++++++++++++++++++++----------------------
> > 2 files changed, 62 insertions(+), 61 deletions(-)
> >
>
> > +++ b/src/lxc/lxc_process.c
> > @@ -769,19 +769,7 @@ int virLXCProcessStop(virLXCDriverPtr driver,
> > }
> >
> > if (priv->cgroup) {
> > - rc = virCgroupKillPainfully(priv->cgroup);
> > - if (rc < 0) {
> > - virReportSystemError(-rc, "%s",
> > - _("Failed to kill container PIDs"));
> > - rc = -1;
> > - goto cleanup;
> > - }
> > - if (rc == 1) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > - _("Some container PIDs refused to die"));
>
> So we previously reported -errno or +1 for two different types of failure...
>
> > - rc = -1;
> > - goto cleanup;
> > - }
> > + virCgroupKillPainfully(priv->cgroup);
>
> ...but now don't report any failure at all. Is this right? Shouldn't
> this be:
>
> if (virCgroupKillPainfully(priv->cgroup) < 0)
> goto cleanup;
>
> or even still distinguish the error message for processes that couldn't
> be killed (but maybe hoisting it into vircgroup.c)?
I'll add in
- virCgroupKillPainfully(priv->cgroup);
+ rc = virCgroupKillPainfully(priv->cgroup);
+ if (rc < 0)
+ return -1;
+ if (rc > 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Some processes refused to die"));
+ return -1;
+ }
> > + ret = virCgroupKillRecursive(group, signum);
> > + VIR_DEBUG("Iteration %zu rc=%d", i, ret);
> > + /* If ret == -1 we hit error, if 0 we ran out of PIDs */
> > + if (ret <= 0)
> > break;
> >
> > usleep(200 * 1000);
> > }
> > - VIR_DEBUG("Complete %d", rc);
> > - return rc;
> > + VIR_DEBUG("Complete %d", ret);
> > + return ret;
>
> Here, you can still return 1 without reporting any error, if you timed out.
Yep, I want to leave it upto the callers to decide if it is an error
condition.
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