dmesg: fix a non working support of /dev/kmesg #177

kreijack · 2025-01-01T17:39:09Z

When I tried to use rust dmesg program, I found that it hangs during a read of /dev/kmsg. Using strace I found few errors.

In order to properly read from /dev/kmsg, 3 things are needed [*]:

open /dev/kmesgs with O_NONBLOCK
handle the EAGAIN/WouldBlock error as end of records
treat '\n' (and not '\0') as record separator.

[*] https://www.kernel.org/doc/Documentation/ABI/testing/dev-kmsg

cakebaker · 2025-01-02T15:10:46Z

Thanks for your PR.

I think the "problem" is that this functionality has not been implemented yet. So far "only" --kmsg-file has been implemented and it uses \0 as a record separator. From the man page:

-K, --kmsg-file file
           Read the /dev/kmsg messages from the given file. Different
           record as expected to be separated by a NULL byte.

And so I think your changes should work alongside the existing functionality instead of replacing it :)

Any additional thoughts @fuad1502 ?

kreijack · 2025-01-02T16:00:39Z

And so I think your changes should work alongside the existing functionality instead of replacing it :)

Ops, I tough (my bad) that even the kmsg format had the same format of /dev/kmsg. Instead it uses \0 as separator instead of \n.
However this should be easily solved (it would be enough to add a separator field to Dmesg, and configure it according.

Let me to to update this patch.

Add the field kmsg_record_separator to the struct Dmesg so it can be changed from \0 to \n when dmesg reads from a file.

fuad1502

Overall, I think it looks good! But please note my comments below. Thank you!

fuad1502 · 2025-01-03T04:04:39Z

src/uu/dmesg/src/dmesg.rs

@@ -196,6 +201,7 @@ impl Dmesg<'_> {
    fn new() -> Self {
        Dmesg {
            kmsg_file: "/dev/kmsg",
+            kmsg_record_separator: 10, // '\n'


You can write it more clearly like so:

Suggested change

kmsg_record_separator: 10, // '\n'

kmsg_record_separator: b'\n',

fuad1502 · 2025-01-03T04:12:17Z

src/uu/dmesg/src/dmesg.rs

@@ -256,10 +262,16 @@ impl Dmesg<'_> {
    }

    fn try_iter(&self) -> UResult<RecordIterator> {
-        let file = File::open(self.kmsg_file)
+        let file = OpenOptions::new()
+            .custom_flags(libc::O_NONBLOCK)


I think we should add conditional compilations for Windows #[cfg(target_os = "windows")] to remove any reference to UNIX specific functionalities. If we're still going to support Windows, we'll probably only support it for reading offline files with --kmsg-file option, therefore not having the non-blocking read would not be an issue.

fuad1502 · 2025-01-03T04:40:36Z

src/uu/dmesg/src/dmesg.rs

@@ -256,10 +262,16 @@ impl Dmesg<'_> {
    }

    fn try_iter(&self) -> UResult<RecordIterator> {
-        let file = File::open(self.kmsg_file)
+        let file = OpenOptions::new()


Cleared /dev/kmsg records, are not shown by dmesg. To simulate this behavior, we need to (quoting from https://www.kernel.org/doc/Documentation/ABI/testing/dev-kmsg):

SEEK_DATA, 0 seek after the last record available at the time the last SYSLOG_ACTION_CLEAR was issued.

I am not sure if there are any Rust library that support seeking with SEEK_DATA, if you can't find it too, you can use libc unsafe method:

let fd = file.as_raw_fd(); unsafe { libc::lseek(fd, 0, libc::SEEK_DATA.into()) };

I updated the patch with your suggestion. I take care to protect the code by #cfg[target_os = "windows"] for the code unix specific.

sylvestre · 2025-01-03T09:02:40Z

src/uu/dmesg/src/dmesg.rs

@@ -256,10 +268,26 @@ impl Dmesg<'_> {
    }

    fn try_iter(&self) -> UResult<RecordIterator> {
-        let file = File::open(self.kmsg_file)
-            .map_err_context(|| format!("cannot open {}", self.kmsg_file))?;
+        let mut oo = OpenOptions::new();


please rename this variable
oo isn't clear

renamed as open_options

sylvestre · 2025-01-03T09:03:08Z

it would be nice to have tests

In order to properly read from /dev/kmsg, we need [*]: 1) open /dev/kmsg with O_NONBLOCK 2) handle the EAGAIN/WouldBlock error as end of records 3) treat '\n' (and not '\0') as record separator. 4) do lseek(fd, 0, SEEK_DATA) Because Windows doesn't support O_NONBLOCK and SEEK_DATA, we had protect these code with #[cfg(not(target_os = "windows"))]. Moreover because Windows doesn't have /dev/kmsg, it is mandatory to use the '-K' switch. [*] https://www.kernel.org/doc/Documentation/ABI/testing/dev-kmsg

kreijack · 2025-01-03T10:27:23Z

it would be nice to have tests

I updated tests in order to avoid failing in windows. However I don't know how can test a correct /dev/kmsg reading.
Some ideas that come to me:

replacing /dev/kmsg with a file and check that dmsg parses the file correctly
adding an hidden options (record separator) to switch between the two formats (the dev/kmsg which uses \n as separator and teh kmsg file format which uses \0 as separator)
injecting some records in kmsg (this requires root privileges), and check that dmesg parse it correctly

Where 1) and 2) will not be able to test O_NONBLOCK and WouldBlock

kreijack · 2025-01-03T10:54:59Z

it would be nice to have tests

I updated tests in order to avoid failing in windows. However I don't know how can test a correct /dev/kmsg reading. Some ideas that come to me:
1. replacing /dev/kmsg with a file and check that dmsg parses the file correctly

2. adding an hidden options (record separator) to switch between the two formats (the dev/kmsg which uses \n as separator and teh kmsg file format which uses \0 as separator)

3. injecting some records in kmsg (this requires root privileges), and check that dmesg parse it correctly
Where 1) and 2) will not be able to test O_NONBLOCK and WouldBlock

The original dmesg, uses the -F/--file options to read from a file instead of /dev/kmsg. But this would be another patches set.

cakebaker · 2025-01-03T13:50:48Z

Can you please run cargo fmt and cargo clippy? There are some minor details the CI complains about.

Because windows build complain if we don't pass a valid -K kmsg_file, pass a kmsg_file in every tests.

kreijack · 2025-01-05T17:53:27Z

Can you please run cargo fmt and cargo clippy? There are some minor details the CI complains about.

I should have correct the last CI warnings

dmesg: add the field record separator to the the struct dmesg

f878075

Add the field kmsg_record_separator to the struct Dmesg so it can be changed from \0 to \n when dmesg reads from a file.

kreijack force-pushed the correct-dmesg branch from a4c0201 to 553e78b Compare January 2, 2025 16:21

fuad1502 suggested changes Jan 3, 2025

View reviewed changes

kreijack force-pushed the correct-dmesg branch from 553e78b to d34cd72 Compare January 3, 2025 08:59

sylvestre reviewed Jan 3, 2025

View reviewed changes

kreijack force-pushed the correct-dmesg branch from d34cd72 to 358f905 Compare January 3, 2025 09:42

kreijack force-pushed the correct-dmesg branch from 358f905 to 193f0c0 Compare January 3, 2025 10:20

kreijack force-pushed the correct-dmesg branch 2 times, most recently from e2672ac to 5cfa874 Compare January 3, 2025 10:34

dmesg: update tests due to /dev/kmsg support

a32aff7

Because windows build complain if we don't pass a valid -K kmsg_file, pass a kmsg_file in every tests.

kreijack force-pushed the correct-dmesg branch from 5cfa874 to a32aff7 Compare January 5, 2025 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dmesg: fix a non working support of /dev/kmesg #177

dmesg: fix a non working support of /dev/kmesg #177

kreijack commented Jan 1, 2025

cakebaker commented Jan 2, 2025

kreijack commented Jan 2, 2025 •

edited

Loading

fuad1502 left a comment

fuad1502 Jan 3, 2025

fuad1502 Jan 3, 2025

fuad1502 Jan 3, 2025

kreijack Jan 3, 2025

sylvestre Jan 3, 2025

kreijack Jan 3, 2025

sylvestre commented Jan 3, 2025

kreijack commented Jan 3, 2025

kreijack commented Jan 3, 2025

cakebaker commented Jan 3, 2025

kreijack commented Jan 5, 2025

	kmsg_record_separator: 10, // '\n'
	kmsg_record_separator: b'\n',

dmesg: fix a non working support of /dev/kmesg #177

Are you sure you want to change the base?

dmesg: fix a non working support of /dev/kmesg #177

Conversation

kreijack commented Jan 1, 2025

cakebaker commented Jan 2, 2025

kreijack commented Jan 2, 2025 • edited Loading

fuad1502 left a comment

Choose a reason for hiding this comment

fuad1502 Jan 3, 2025

Choose a reason for hiding this comment

fuad1502 Jan 3, 2025

Choose a reason for hiding this comment

fuad1502 Jan 3, 2025

Choose a reason for hiding this comment

kreijack Jan 3, 2025

Choose a reason for hiding this comment

sylvestre Jan 3, 2025

Choose a reason for hiding this comment

kreijack Jan 3, 2025

Choose a reason for hiding this comment

sylvestre commented Jan 3, 2025

kreijack commented Jan 3, 2025

kreijack commented Jan 3, 2025

cakebaker commented Jan 3, 2025

kreijack commented Jan 5, 2025

kreijack commented Jan 2, 2025 •

edited

Loading