On Mon 2020-02-17 12:13:25, John Ogness wrote: > On 2020-02-14, Petr Mladek <pmladek@xxxxxxxx> wrote: > >> I oversaw that devkmsg_open() setup a printk_record and so I did not > >> see to add the extra NULL initialization of text_line_count. There > >> should be be an initializer function/macro to avoid this danger. > >> > >> John Ogness > >> > >> The quick fixup: > >> > >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > >> index d0d24ee1d1f4..5ad67ff60cd9 100644 > >> --- a/kernel/printk/printk.c > >> +++ b/kernel/printk/printk.c > >> @@ -883,6 +883,7 @@ static int devkmsg_open(struct inode *inode, struct file *file) > >> user->record.text_buf_size = sizeof(user->text_buf); > >> user->record.dict_buf = &user->dict_buf[0]; > >> user->record.dict_buf_size = sizeof(user->dict_buf); > >> + user->record.text_line_count = NULL; > > > > The NULL pointer hidden in the structure also complicates the code > > reading. It is less obvious when the same function is called > > only to get the size/count and when real data. > > OK. > > > I played with it and created extra function to get this information. > > > > In addition, I had problems to follow the code in > > record_print_text_inline(). So I tried to reuse the new function > > and the existing record_printk_text() there. > > > > Please, find below a patch that I ended with. I booted a system > > with this patch. But I guess that I actually did not use the > > record_print_text_inline(). So, it might be buggy. > > Yes, there are several bugs. But I see where you want to go with this: > > - introduce prb_count_lines() to handle line counting > > - introduce prb_read_valid_info() for only reading meta-data and getting > the line count > > - also use prb_count_lines() internally In addition, I would like share the code between record_print_text_inline() and record_print_text(). They both do very similar thing and the logic in far from trivial. Alternative solution would be to get rid of record_print_text() and use record_print_text_inline() everywhere. It will have some advantages: + _inline() variant will get real testing + no code duplication + saving the extra buffer also in console, sysfs, and devkmsg interface. > I will include these changes in v2. I will still introduce the static > inlines to initialize records because readers and writers do it > differently. Sounds good. Best Regards, Petr _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
