04 task struct#126
Conversation
marynamalakhova
force-pushed
the
04_task_struct
branch
5 times, most recently
from
8557de9 to
8460748
Compare
04_basic_struct/Makefile
Outdated
| @@ -0,0 +1,7 @@ | |||
| TARGET = bstructmodule | |||
There was a problem hiding this comment.
The task says Implement object with name “hello”
04_basic_struct/Makefile
Outdated
| @@ -0,0 +1,7 @@ | |||
| TARGET = bstructmodule | |||
| obj-m+=$(TARGET).o | |||
| #include <linux/init.h> // Macros used to mark up functions __init __exit | ||
| #include <linux/module.h> // Core header for loading LKMs into the kernel |
04_basic_struct/bstructmodule.c
Outdated
| sscanf(buf, "%d\n", &value); | ||
| pr_info("hello_store: value = %d\n", value); | ||
| return count; |
There was a problem hiding this comment.
please add error handling
04_basic_struct/bstructmodule.c
Outdated
| if (!hello_kobj) | ||
| return -ENOMEM; | ||
| /* Create the files associated with this kobject */ | ||
| res = sysfs_create_group(hello_kobj, &attr_group); |
There was a problem hiding this comment.
What's the purpose of using group API for a single attribute?
04_basic_struct/bstructmodule.c
Outdated
| struct list_head head_list; | ||
| }; | ||
|
|
||
| LIST_HEAD(linkedlist); |
There was a problem hiding this comment.
It's usually bad idea to define global variables with such generic name.
Please limit its scope.
04_basic_struct/bstructmodule.c
Outdated
| if (new_node->node == NULL) | ||
| return -ENOMEM; | ||
| list_add_tail(&new_node->head_list, &linkedlist); | ||
| pr_info("store nodes: Added node = %s", buf); |
There was a problem hiding this comment.
It's not a big deal, but prefer referencing actually added node instead of user buffer.
Also please don't forget newline explicit character for complete messages. (in other places as well)
There was a problem hiding this comment.
kstrdup allocates space for and copy an existing string. That's why nodes have been already stored with the end of the string. When we are printing out the nodes extra newline explicit character seems redundant
| list_for_each_entry(cur_node, &linkedlist, head_list) { | ||
| res = scnprintf(buf+offset, PAGE_SIZE-offset, "%s", cur_node->node); | ||
| offset += res; | ||
| } |
There was a problem hiding this comment.
IMO it makes sense to somehow separate entries in the output.
| /mnt # | ||
| /mnt # lsmod | ||
| /mnt # | ||
|
|
There was a problem hiding this comment.
Please don't mix implementation and test logs in the same commit.
AleksandrBulyshchenko
added
Change requested
marynamalakhova
force-pushed
the
04_task_struct
branch
2 times, most recently
from
000b485 to
decfb19
Compare
Added simple module that provides basic operations with /sys/kernel/hello/list Variable BUILD_KERNEL should be set in the environment for building BUILD_KERNEL should define path to the linux kernel source folder Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>
marynamalakhova
force-pushed
the
04_task_struct
branch
2 times, most recently
from
6a20fcb to
ec4e981
Compare
Add list of strings insted of int value Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>
Add BBB log Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>
marynamalakhova
force-pushed
the
04_task_struct
branch
from
ec4e981 to
9c710d9
Compare
marynamalakhova
added
Ready for review
|
Thank you for the review, I've fixed |
3 participants
Module has been created that could store parameters list in /sys/kernel/hello/list