[RFC] Fix the kmod internal list behavior?

[Yang Chengwei]

Hi List,

I just found that there are some minor issues in kmod iternal list
implementation implemented by libkmod-list.c.

Say
1. list_node_append() is identical with list_node_insert_before(),
so somehow the list_node_append() in fact does "prepend" operation.

2. just the same as list_node_append(), kmod_list_append() which
invokes the former just do "prepend" operation rather than "append".

I'd like to fix these internal APIs to do the operation suggested by its
name like
1. drop list_node_append()
2. call list_node_insert_after() to do "append" operation
3. call list_mode_insert_before() to do "prepend" operation

Since the list here is circular linked, so I think these changes should
not break things except the test cases.

Any comments are appreciated.

--
Thanks,
Chengwei

[Lucas De Marchi]

Post by Yang Chengwei
Hi List,
I just found that there are some minor issues in kmod iternal list
implementation implemented by libkmod-list.c.
Say
1. list_node_append() is identical with list_node_insert_before(),
so somehow the list_node_append() in fact does "prepend" operation.

To append to a circular list, you insert it *before* the first node.
So what's wrong here?

Post by Yang Chengwei
2. just the same as list_node_append(), kmod_list_append() which
invokes the former just do "prepend" operation rather than "append".

like I said, it's expected, since it's circular. kmod_list_append()
expect you to pass the head of the list, so to append to the list you
prepend to the first node, but you don't return it as the new head
unless it's the first one - check the return code.

Post by Yang Chengwei
I'd like to fix these internal APIs to do the operation suggested by its
name like
1. drop list_node_append()
2. call list_node_insert_after() to do "append" operation
3. call list_mode_insert_before() to do "prepend" operation
Since the list here is circular linked, so I think these changes should
not break things except the test cases.
Any comments are appreciated.

Are you analyzing the source code or are you seeing a real problem?
It'd be nice to write a small test showing the problem - you can use
the testsuite for this... take test-alias as an example (i.e. you need
to link to the private lib).

regards
Lucas De Marchi

[Yang Chengwei]

Post by Lucas De Marchi

Post by Yang Chengwei
Hi List,
I just found that there are some minor issues in kmod iternal list
implementation implemented by libkmod-list.c.
Say
1. list_node_append() is identical with list_node_insert_before(),
so somehow the list_node_append() in fact does "prepend" operation.

To append to a circular list, you insert it *before* the first node.
So what's wrong here?

Yeap, it's definitely right. I was misunderstanding. :-(

Post by Lucas De Marchi

Post by Yang Chengwei
2. just the same as list_node_append(), kmod_list_append() which
invokes the former just do "prepend" operation rather than "append".

like I said, it's expected, since it's circular. kmod_list_append()
expect you to pass the head of the list, so to append to the list you
prepend to the first node, but you don't return it as the new head
unless it's the first one - check the return code.

Post by Yang Chengwei
I'd like to fix these internal APIs to do the operation suggested by its
name like
1. drop list_node_append()
2. call list_node_insert_after() to do "append" operation
3. call list_mode_insert_before() to do "prepend" operation
Since the list here is circular linked, so I think these changes should
not break things except the test cases.
Any comments are appreciated.

Are you analyzing the source code or are you seeing a real problem?

Relax, no real issue found.

Post by Lucas De Marchi
It'd be nice to write a small test showing the problem - you can use

So I think it's not necessary to add a dedicate test for list, the
others somehow use the list, like modinfo test.

--
Thanks,
Chengwei

Post by Lucas De Marchi
the testsuite for this... take test-alias as an example (i.e. you need
to link to the private lib).
regards
Lucas De Marchi

[Lucas De Marchi]

Post by Yang Chengwei

Post by Lucas De Marchi

Post by Yang Chengwei
Hi List,
I just found that there are some minor issues in kmod iternal list
implementation implemented by libkmod-list.c.
Say
1. list_node_append() is identical with list_node_insert_before(),
so somehow the list_node_append() in fact does "prepend" operation.

To append to a circular list, you insert it *before* the first node.
So what's wrong here?

Yeap, it's definitely right. I was misunderstanding. :-(

Post by Lucas De Marchi

Post by Yang Chengwei
2. just the same as list_node_append(), kmod_list_append() which
invokes the former just do "prepend" operation rather than "append".

like I said, it's expected, since it's circular. kmod_list_append()
expect you to pass the head of the list, so to append to the list you
prepend to the first node, but you don't return it as the new head
unless it's the first one - check the return code.

Post by Yang Chengwei
I'd like to fix these internal APIs to do the operation suggested by its
name like
1. drop list_node_append()
2. call list_node_insert_after() to do "append" operation
3. call list_mode_insert_before() to do "prepend" operation
Since the list here is circular linked, so I think these changes should
not break things except the test cases.
Any comments are appreciated.

Are you analyzing the source code or are you seeing a real problem?

Relax, no real issue found.

Good to know :-)

Post by Yang Chengwei

Post by Lucas De Marchi
It'd be nice to write a small test showing the problem - you can use

So I think it's not necessary to add a dedicate test for list, the
others somehow use the list, like modinfo test.

On a side note about the current list implementation: one interesting
thing to investigate is how much memory we waste on list nodes and how
we could improve that. One possible answer is by using inlined nodes
like kernel's.

In some places we use kmod_list very heavily like in depmod. Last time
I checked I remember a great portion of the memory used just to store
the list nodes. If you would like to improve this part, or at least
profile it, it would be very nice :)


Lucas De Marchi

[Yang Chengwei]

Post by Lucas De Marchi

Post by Yang Chengwei

Post by Lucas De Marchi

Post by Yang Chengwei
Hi List,
I just found that there are some minor issues in kmod iternal list
implementation implemented by libkmod-list.c.
Say
1. list_node_append() is identical with list_node_insert_before(),
so somehow the list_node_append() in fact does "prepend" operation.

To append to a circular list, you insert it *before* the first node.
So what's wrong here?

Yeap, it's definitely right. I was misunderstanding. :-(

Post by Lucas De Marchi

Post by Yang Chengwei
2. just the same as list_node_append(), kmod_list_append() which
invokes the former just do "prepend" operation rather than "append".

like I said, it's expected, since it's circular. kmod_list_append()
expect you to pass the head of the list, so to append to the list you
prepend to the first node, but you don't return it as the new head
unless it's the first one - check the return code.

Post by Yang Chengwei
I'd like to fix these internal APIs to do the operation suggested by its
name like
1. drop list_node_append()
2. call list_node_insert_after() to do "append" operation
3. call list_mode_insert_before() to do "prepend" operation
Since the list here is circular linked, so I think these changes should
not break things except the test cases.
Any comments are appreciated.

Are you analyzing the source code or are you seeing a real problem?

Relax, no real issue found.

Good to know :-)

Post by Yang Chengwei

Post by Lucas De Marchi
It'd be nice to write a small test showing the problem - you can use

So I think it's not necessary to add a dedicate test for list, the
others somehow use the list, like modinfo test.

On a side note about the current list implementation: one interesting
thing to investigate is how much memory we waste on list nodes and how
we could improve that. One possible answer is by using inlined nodes
like kernel's.
In some places we use kmod_list very heavily like in depmod. Last time
I checked I remember a great portion of the memory used just to store
the list nodes. If you would like to improve this part, or at least
profile it, it would be very nice :)

Add to my TODO list, but I'm not sure what the ETA is. :-)

--
Thanks,
Chengwei

Post by Lucas De Marchi
Lucas De Marchi