[lkml]   [2008]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [RFC v10][PATCH 09/13] Restore open file descriprtors
    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);

    static void cr_obj_ref_grab(struct cr_objref *obj)
    switch (obj->type) {
    case CR_OBJ_FILE:
    get_file((struct file *) obj->ptr);

    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;

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

    And the add operation becomes:

    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

    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

     \ /
      Last update: 2008-12-02 02:35    [W:0.027 / U:2.720 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site