Re: [RFC v10][PATCH 09/13] Restore open file descriprtors

On Mon, 2008-12-01 at 13:07 -0800, Dave Hansen wrote:

quoted

When a shared object is inserted to the hash we automatically take another
reference to it (according to its type) for as long as it remains in the
hash. See:  'cr_obj_ref_grab()' and 'cr_obj_ref_drop()'.  So by moving that
call higher up, we protect the struct file.

That's kinda (and by kinda I mean really) disgusting.  Hiding that two
levels deep in what is effectively the hash table code where no one will
ever see it is really bad.  It also makes you lazy thinking that the
hash code will just know how to take references on whatever you give to
it.

I think cr_obj_ref_grab() is hideous obfuscation and needs to die.
Let's just do the get_file() directly, please.

Well, I at least see why you need it now.  The objhash thing is trying
to be a pretty generic hash implementation and it does need to free the
references up when it is destroyed.  Instead of keeping a "hash of
files" and a "hash of pipes" or other shared objects, there's just a
single hash for everything.

One alternative here would be to have an ops-style release function that
gets called instead of what we have now:
        
        static void cr_obj_ref_drop(struct cr_objref *obj)
        {
                switch (obj->type) {
                case CR_OBJ_FILE:
                        fput((struct file *) obj->ptr);
                        break;
                default:
                        BUG();
                }
        }
        
        static void cr_obj_ref_grab(struct cr_objref *obj)
        {
                switch (obj->type) {
                case CR_OBJ_FILE:
                        get_file((struct file *) obj->ptr);
                        break;
                default:
                        BUG();
                }
        }

That would make it something like:

struct cr_obj_ops {
	int type;
	void (*release)(struct cr_objref *obj);
};

void cr_release_file(struct cr_objref *obj)
{
	struct file *file = obj->ptr;
	put_file(file);
}

struct cr_obj_ops cr_file_ops = {
	.type = CR_OBJ_FILE,
	.release = cr_release_file,
};


And the add operation becomes:

	get_file(file);
	new = cr_obj_add_ptr(ctx, file, &objref, &cr_file_ops, 0);

with 'cr_file_ops' basically replacing the CR_OBJ_FILE that got passed
before.

I like that because it only obfuscates what truly needs to be abstracted
out: the release side.  Hiding that get_file() is really tricky.

But, I guess we could also just kill cr_obj_ref_grab(), do the
get_file() explicitly and still keep cr_obj_ref_drop() as it is now.

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html