lkml.org 
[lkml]   [2010]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 8/19]: SCST SYSFS interface implementation
Greg KH, on 10/22/2010 09:56 PM wrote:
> On Fri, Oct 22, 2010 at 09:30:53PM +0400, Vladislav Bolkhovitin wrote:
>> + unsigned int tgt_kobj_initialized:1;
>
> It's the middle of the merge window, and I'm about to go on vacation, so
> I didn't read this patch after this line.
>
> It's obvious that this patch is wrong, you shouldn't need to worry about
> this. And even if you did, you don't need this flag.
>
> Why are you trying to do something that no one else needs? Why make
> things harder than they have to be.

I tried to explain that to you in http://lkml.org/lkml/2010/10/14/291
and mentioned there the need to create this flag to track
half-initialized kobjects. You agreed
(http://lkml.org/lkml/2010/10/14/299) that don't return half-initialized
objects is a regular kernel practice, but then requested to strictly
bound the larger object freeing to its kobject release(), which means
that all SYSFS creating functions now have to return half-initialized
SYSFS hierarchy in case of any error. Hence the flag to track it.

Simply, any SCST object has a lot of other things to initialize besides
its kobject, hence the need either to free it independently from its
kobject, or track by a flag if its kobjects initialized.

For illustration, here is the simplified code from my previous example,
i.e. without this patch. I added to it scst_unregister_target() to make
it more complete.

void scst_tgt_sysfs_del(struct scst_tgt *tgt)
{
...
kobject_put(&tgt->tgt_kobj);
wait_for_completion(&tgt->tgt_kobj_release_cmpl);
}

int scst_tgt_sysfs_create(struct scst_tgt *tgt)
{
init_completion(&tgt->tgt_kobj_release_cmpl);

res = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype,
&tgt->tgtt->tgtt_kobj, tgt->tgt_name);
if (res != 0)
goto out;

res = sysfs_create_file(&tgt->tgt_kobj,
&tgt_enable_attr.attr);
if (res != 0)
goto out_err;
...
out:
return res;

out_err:
scst_tgt_sysfs_del(tgt);
goto out;
}

struct scst_tgt *scst_register_target()
{
struct scst_tgt *tgt;
int rc = 0;

rc = scst_alloc_tgt();
if (rc != 0)
goto out;
...
tgt->tgt_name = kmalloc(strlen(target_name) + 1, GFP_KERNEL);
if (tgt->tgt_name == NULL)
goto out_free_tgt;
...

mutex_lock();

rc = scst_tgt_sysfs_create(tgt);
if (rc < 0)
goto out_unlock;

tgt->default_acg = scst_alloc_add_acg();
if (tgt->default_acg == NULL)
goto out_sysfs_del;
...

out:
return tgt;

out_sysfs_del:
mutex_unlock();
scst_tgt_sysfs_del(tgt)
goto out_free_tgt;

out_unlock:
mutex_unlock();

out_free_tgt:
scst_free_tgt(tgt);
}

void scst_unregister_target(struct scst_tgt *tgt)
{
...

scst_tgt_sysfs_del(tgt);
...
scst_free_tgt(tgt);
}

You can see complete source code of those functions in the original
patch set I sent in this thread (patches 4 and 8).

What am I missing and how errors processing should be done with neither
freeing tgt not in tgt_kobj release() as above, nor without
tgt_kobj_initialized flag as in the patch?

Thanks,
Vlad


\
 
 \ /
  Last update: 2010-10-22 20:45    [W:0.111 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site