On Mon, Jan 04, 2016 at 05:33:45PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 04, 2016 at 10:18:50AM +0000, Chris Wilson wrote:
> > On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> > > The current error path for failure when establishing a handle for a GEM
> > > object is unbalance, e.g. we call object_close() without calling first
> > > object_open(). Use the typical onion structure to only undo what has
> > > been set up prior to the error.
> > >
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > ---
> > > drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> > > 1 file changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 2e10bba4468b..a08176debc0e 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file
> > > *file_priv,
> > > spin_unlock(&file_priv->table_lock);
> > > idr_preload_end();
> > > mutex_unlock(&dev->object_name_lock);
> > > - if (ret < 0) {
> > > - drm_gem_object_handle_unreference_unlocked(obj);
> > > - return ret;
> > > - }
> > > + if (ret < 0)
> > > + goto err_unref;
> > > +
> > > *handlep = ret;
> > >
> > > ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> > > - if (ret) {
> > > - drm_gem_handle_delete(file_priv, *handlep);
> > > - return ret;
> > > - }
> > > + if (ret)
> > > + goto err_remove;
> > >
> > > if (dev->driver->gem_open_object) {
> > > ret = dev->driver->gem_open_object(obj, file_priv);
> > > - if (ret) {
> > > - drm_gem_handle_delete(file_priv, *handlep);
> > > - return ret;
> > > - }
> > > + if (ret)
> > > + goto err_revoke;
> > > }
> > >
> > > return 0;
> > > +
> > > +err_revoke:
> > > + drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> > > +err_remove:
> > > + spin_lock(&file_priv->table_lock);
> > > + idr_remove(&file_priv->object_idr, *handlep);
> >
> > For bonus points, we could *handlep = 0; here.
>
> For these sort of things I usually prefer to not clobber any
> return parameters unless the whole operation suceeds.
True, resetting handlep back to the INVALID_HANDLE value was just a step
in the right direction ;-)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
