From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Hansen Subject: Re: [RFC v10][PATCH 09/13] Restore open file descriprtors Date: Mon, 01 Dec 2008 17:31:08 -0800 Message-ID: <1228181468.2971.146.camel@nimitz> References: <1227747884-14150-1-git-send-email-orenl@cs.columbia.edu> <1227747884-14150-10-git-send-email-orenl@cs.columbia.edu> <20081128112745.GR28946@ZenIV.linux.org.uk> <1228159324.2971.74.camel@nimitz> <49344C11.6090204@cs.columbia.edu> <1228164873.2971.95.camel@nimitz> <49345086.4@cs.columbia.edu> <1228165651.2971.99.camel@nimitz> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1228165651.2971.99.camel@nimitz> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Oren Laadan Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linus Torvalds , Thomas Gleixner , Al Viro , "H. Peter Anvin" , Ingo Molnar , Andrew Morton List-Id: linux-api@vger.kernel.org On Mon, 2008-12-01 at 13:07 -0800, Dave Hansen wrote: > > 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