Vladimir Saveliev wrote:
>
>
>>>>+/*
>>>>+ * Initialization stages for reiser4.
>>>>+ *
>>>>+ * These enumerate various things that have to be done during reiser4
>>>>+ * startup. Initialization code (init_reiser4()) keeps track of what stage
>>>>was
>>>>+ * reached, so that proper undo can be done if error occurs during
>>>>+ * initialization.
>>>>+ */
>>>>+typedef enum {
>>>>+ INIT_NONE, /* nothing is initialized yet */
>>>>+ INIT_INODECACHE, /* inode cache created */
>>>>+ INIT_CONTEXT_MGR, /* list of active contexts created */
>>>>+ INIT_ZNODES, /* znode slab created */
>>>>+ INIT_PLUGINS, /* plugins initialized */
>>>>+ INIT_PLUGIN_SET, /* psets initialized */
>>>>+ INIT_TXN, /* transaction manager initialized */
>>>>+ INIT_FAKES, /* fake inode initialized */
>>>>+ INIT_JNODES, /* jnode slab initialized */
>>>>+ INIT_EFLUSH, /* emergency flush initialized */
>>>>+ INIT_FQS, /* flush queues initialized */
>>>>+ INIT_DENTRY_FSDATA, /* dentry_fsdata slab initialized */
>>>>+ INIT_FILE_FSDATA, /* file_fsdata slab initialized */
>>>>+ INIT_D_CURSOR, /* d_cursor suport initialized */
>>>>+ INIT_FS_REGISTERED, /* reiser4 file system type registered */
>>>>+} reiser4_init_stage;
>>>>
>>>>
>>>>
>>>>
>>>Please use regular gotos instead. This is a silly hack especially since you
>>>don't have release function for all of the stages.
>>>
>>>
>>>
>>>
>>I'll let vs comment.
>>
>>
>>
>
>IMHO, it is convinient. But lets change it as requested.
>
>
No, if you like it, say so and it stays.
>>>>+ *
>>>>+ * kmalloc/kfree leak detection: reiser4_kmalloc(), reiser4_kfree(),
>>>>+ * reiser4_kfree_in_sb().
>>>>
>>>>
>>>>
>>>>
>>>Please don't do this! We've had enough trouble trying to get the
>>>current subsystem specific allocators out, so do not introduce a new one. If
>>>you need memory leak detection, make it generic and submit that. Reiser4,
>>>like
>>>all other subsystems, should use kmalloc() and friends directly.
>>>
>>>
>>>
>>>
>>I will let vs comment.
>>
>>
>>
>I agree with Pekka.
>
>
Ok, can you make it generic easily?
>
>
>>>
>>>
>>>
>>>
>>>>--- /dev/null 2003-09-23 21:59:22.000000000 +0400
>>>>+++ linux-2.6.11-vs/fs/reiser4/debug.h 2005-06-03 17:49:38.297184283
>>>>+0400
>>>>+
>>>>+/*
>>>>+ * Error code tracing facility. (Idea is borrowed from XFS code.)
>>>>
>>>>
>>>>
>>>>
>>>Could this be turned into a generic facility?
>>>
>>>
>>>
>
>I do not think that it will ever become accepted.
>To get use of that task_t would have to be extended with fields to store
>error code, file name and line in it, and several return addresses.
>Moreover lines like
>return -ENOENT;
>would have to be replaced with:
>return RETERR(-ENOENT);
>
>This is debugging feature, we should probably move it to our internal
>debugging patch.
>
>
>
>>>
>>>
>>>
>>>
>>>>+#define reiser4_internal
>>>>
>>>>
>>>>
>>>>
>>>Please drop the above useless #define.
>>>
>>>
>>>
>
>ok
>
>
>
>>>>--- /dev/null 2003-09-23 21:59:22.000000000 +0400
>>>>+++ linux-2.6.11-vs/fs/reiser4/init_super.c 2005-06-03 17:51:27.959201950
>>>>+0400
>>>>+
>>>>+#define _INIT_PARAM_LIST (struct super_block * s, reiser4_context * ctx,
>>>>void * data, int silent)
>>>>+#define _DONE_PARAM_LIST (struct super_block * s)
>>>>+
>>>>+#define _INIT_(subsys) static int _init_##subsys _INIT_PARAM_LIST
>>>>+#define _DONE_(subsys) static void _done_##subsys _DONE_PARAM_LIST
>>>>
>>>>
>>>>
>>>>
>>>Please remove this macro obfuscation.
>>>
>>>
>>>
>>>
>>vs, I think he is right, do you?
>>
>>
>>
>>>
>>>
>>>
>>>
>>>>+
>>>>+_DONE_EMPTY(exit_context)
>>>>+
>>>>+struct reiser4_subsys {
>>>>+ int (*init) _INIT_PARAM_LIST;
>>>>+ void (*done) _DONE_PARAM_LIST;
>>>>+};
>>>>+
>>>>+#define _SUBSYS(subsys) {.init = &_init_##subsys, .done = &_done_##subsys}
>>>>+static struct reiser4_subsys subsys_array[] = {
>>>>+ _SUBSYS(mount_flags_check),
>>>>+ _SUBSYS(sinfo),
>>>>+ _SUBSYS(context),
>>>>+ _SUBSYS(parse_options),
>>>>+ _SUBSYS(object_ops),
>>>>+ _SUBSYS(read_super),
>>>>+ _SUBSYS(tree0),
>>>>+ _SUBSYS(txnmgr),
>>>>+ _SUBSYS(ktxnmgrd_context),
>>>>+ _SUBSYS(ktxnmgrd),
>>>>+ _SUBSYS(entd),
>>>>+ _SUBSYS(formatted_fake),
>>>>+ _SUBSYS(disk_format),
>>>>+ _SUBSYS(sb_counters),
>>>>+ _SUBSYS(d_cursor),
>>>>+ _SUBSYS(fs_root),
>>>>+ _SUBSYS(safelink),
>>>>+ _SUBSYS(exit_context)
>>>>+};
>>>>
>>>>
>>>>
>>>>
>>>The above is overkill and silly. parse_options and read_super, for
>>>example, are _not_ a subsystem inits but regular fs ops. Please consider
>>>dropping this altogether but at least trim it down to something sane.
>>>
>>>
>>>
>
>Pekka, would you prefer something like:
>
>reiser4_fill_super()
>{
> if (init_a() == 0) {
> if (init_b() == 0) {
> if (init_c() == 0) {
> if (init_last() == 0)
> return 0;
> else {
> done_c();
> done_b();
> done_a();
> }
> } else {
> done_b();
> done_a();
> }
> } else {
> done_a();
> }
> }
>}
>
>
I think the above is easier to read than the below. Macros can obscure
sometimes, and one of our weaknesses is a tendency to use macros in such
a way that it frustrates meta-. use in emacs. Nikita did however
mention that there was something that could understand macros when
constructing tags files, but I forgot what that was.
>With these macros we have reiser4_fill_super to look like the below, and
>it remains unchanged when something new is added for initilizing on
>filesystem mounting.
>
>reiser4_fill_super()
>{
> for (i = 0; i < REISER4_NR_SUBSYS; i++) {
> ret = subsys_array[i].init(s, &ctx, data, silent);
> if (ret) {
> done_super(s, i - 1);
> return ret;
> }
> }
>}
>
>
>
>
>
>
