Re: [PATCH] printk.c: removed unnecessary code
On Wed 2016-11-30 10:14:28, Michal Hocko wrote: > [Resending with the full CC list as my email client has clobbered it in > the previous attempt for some reason] > > On Tue 29-11-16 16:19:01, Sebastian Duda wrote: > > snprintf((char *) ?, 0, ...); always returns Zero and doesn't change the > > data. > > Thus the execution of > > snprintf(NULL, 0, "[%5lu.00] ", (unsigned long)ts); > > has no effect on program. > > The substitution with 0 increases the readability of the code. > > Are you sure this is correct. As per vsnprintf documentation: > " > * The return value is the number of characters which would > * be generated for the given input, excluding the trailing > * '\0', as per ISO C99. > " > > this should just work as 35dac27cedd1 ("printk: fix incorrect length > from print_time() when seconds > 9") intended. > > I haven't checked the implementation though so I might be wrong here. print_time() actually is not used by printk() directly. Therefore the above reasoning does not apply. They key here is to look when the buf might be NULL. It is when we try to get an idea how much space will be needed for the message when it is printed to some output device, e.g. console or syslog. For example, there is the following chain of calls: syslog_print_all() msg_print_text(msg, prev, true, NULL, 0) print_prefix(msg, syslog, NULL) len += print_time(msg->ts_nsec, buf ? buf + len : NULL) It means that we really need to know the length of the printed string even when we do not write it to a buffer for the moment. By other words, the patch would break the functionality. > > > > Signed-off-by: Sebastian Duda> > Signed-off-by: Tobias Baumeister Nacked-by: Petr Mladek Best Regards, Petr
Re: [PATCH] printk.c: removed unnecessary code
On Wed 2016-11-30 10:14:28, Michal Hocko wrote: > [Resending with the full CC list as my email client has clobbered it in > the previous attempt for some reason] > > On Tue 29-11-16 16:19:01, Sebastian Duda wrote: > > snprintf((char *) ?, 0, ...); always returns Zero and doesn't change the > > data. > > Thus the execution of > > snprintf(NULL, 0, "[%5lu.00] ", (unsigned long)ts); > > has no effect on program. > > The substitution with 0 increases the readability of the code. > > Are you sure this is correct. As per vsnprintf documentation: > " > * The return value is the number of characters which would > * be generated for the given input, excluding the trailing > * '\0', as per ISO C99. > " > > this should just work as 35dac27cedd1 ("printk: fix incorrect length > from print_time() when seconds > 9") intended. > > I haven't checked the implementation though so I might be wrong here. print_time() actually is not used by printk() directly. Therefore the above reasoning does not apply. They key here is to look when the buf might be NULL. It is when we try to get an idea how much space will be needed for the message when it is printed to some output device, e.g. console or syslog. For example, there is the following chain of calls: syslog_print_all() msg_print_text(msg, prev, true, NULL, 0) print_prefix(msg, syslog, NULL) len += print_time(msg->ts_nsec, buf ? buf + len : NULL) It means that we really need to know the length of the printed string even when we do not write it to a buffer for the moment. By other words, the patch would break the functionality. > > > > Signed-off-by: Sebastian Duda > > Signed-off-by: Tobias Baumeister Nacked-by: Petr Mladek Best Regards, Petr
Re: [PATCH] printk.c: removed unnecessary code
[Resending with the full CC list as my email client has clobbered it in the previous attempt for some reason] On Tue 29-11-16 16:19:01, Sebastian Duda wrote: > snprintf((char *) ?, 0, ...); always returns Zero and doesn't change the data. > Thus the execution of > snprintf(NULL, 0, "[%5lu.00] ", (unsigned long)ts); > has no effect on program. > The substitution with 0 increases the readability of the code. Are you sure this is correct. As per vsnprintf documentation: " * The return value is the number of characters which would * be generated for the given input, excluding the trailing * '\0', as per ISO C99. " this should just work as 35dac27cedd1 ("printk: fix incorrect length from print_time() when seconds > 9") intended. I haven't checked the implementation though so I might be wrong here. > > Signed-off-by: Sebastian Duda> Signed-off-by: Tobias Baumeister > --- > kernel/printk/printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 5028f4f..fe3fec1 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1186,7 +1186,7 @@ static size_t print_time(u64 ts, char *buf) > rem_nsec = do_div(ts, 10); > > if (!buf) > - return snprintf(NULL, 0, "[%5lu.00] ", (unsigned long)ts); > + return 0; > > return sprintf(buf, "[%5lu.%06lu] ", > (unsigned long)ts, rem_nsec / 1000); > -- > 2.7.4 -- Michal Hocko SUSE Labs
Re: [PATCH] printk.c: removed unnecessary code
[Resending with the full CC list as my email client has clobbered it in the previous attempt for some reason] On Tue 29-11-16 16:19:01, Sebastian Duda wrote: > snprintf((char *) ?, 0, ...); always returns Zero and doesn't change the data. > Thus the execution of > snprintf(NULL, 0, "[%5lu.00] ", (unsigned long)ts); > has no effect on program. > The substitution with 0 increases the readability of the code. Are you sure this is correct. As per vsnprintf documentation: " * The return value is the number of characters which would * be generated for the given input, excluding the trailing * '\0', as per ISO C99. " this should just work as 35dac27cedd1 ("printk: fix incorrect length from print_time() when seconds > 9") intended. I haven't checked the implementation though so I might be wrong here. > > Signed-off-by: Sebastian Duda > Signed-off-by: Tobias Baumeister > --- > kernel/printk/printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 5028f4f..fe3fec1 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1186,7 +1186,7 @@ static size_t print_time(u64 ts, char *buf) > rem_nsec = do_div(ts, 10); > > if (!buf) > - return snprintf(NULL, 0, "[%5lu.00] ", (unsigned long)ts); > + return 0; > > return sprintf(buf, "[%5lu.%06lu] ", > (unsigned long)ts, rem_nsec / 1000); > -- > 2.7.4 -- Michal Hocko SUSE Labs