Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()
On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote: > Move the non-trivial code from the long pointer() function. We are > going > to add a check for the access to the address that will make it even > more > complicated. > > Also it is better to warn about unknown specifier instead of falling > back to the %p behavior. It will help people to understand what is > going wrong. They expect the IP address and not a pointer anyway > in this situation. > + WARN_ONCE(1, "Unsupported pointer format specifier: > %%p%c%c\n", > + fmt[0], fmt[1]); > I think WARN is too much here. pr_warn_once() ? -- Andy ShevchenkoIntel Finland Oy
Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()
On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote: > Move the non-trivial code from the long pointer() function. We are > going > to add a check for the access to the address that will make it even > more > complicated. > > Also it is better to warn about unknown specifier instead of falling > back to the %p behavior. It will help people to understand what is > going wrong. They expect the IP address and not a pointer anyway > in this situation. > + WARN_ONCE(1, "Unsupported pointer format specifier: > %%p%c%c\n", > + fmt[0], fmt[1]); > I think WARN is too much here. pr_warn_once() ? -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()
On Thu 2018-04-05 08:58:16, Sergey Senozhatsky wrote: > On (04/04/18 10:58), Petr Mladek wrote: > > > > Also it is better to warn about unknown specifier instead of falling > > back to the %p behavior. It will help people to understand what is > > going wrong. They expect the IP address and not a pointer anyway > > in this situation. > > > > May be. If one sees a hashed value where IP address/device name/etc > was meant to be then it's already a sign that something is wrong. > Those WARN_ONCE that you have added make things simpler, I agree. > A quick question, what happens on !CONFIG_BUG systems (where we have > no_printk() WARN)? People with CONFIG_BUG disabled will miss much more important warnings. IMHO, an unreported typo in printk format will be one of their smallest problems. Best Regards, Petr
Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()
On Thu 2018-04-05 08:58:16, Sergey Senozhatsky wrote: > On (04/04/18 10:58), Petr Mladek wrote: > > > > Also it is better to warn about unknown specifier instead of falling > > back to the %p behavior. It will help people to understand what is > > going wrong. They expect the IP address and not a pointer anyway > > in this situation. > > > > May be. If one sees a hashed value where IP address/device name/etc > was meant to be then it's already a sign that something is wrong. > Those WARN_ONCE that you have added make things simpler, I agree. > A quick question, what happens on !CONFIG_BUG systems (where we have > no_printk() WARN)? People with CONFIG_BUG disabled will miss much more important warnings. IMHO, an unreported typo in printk format will be one of their smallest problems. Best Regards, Petr
Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()
On (04/04/18 10:58), Petr Mladek wrote: > > Also it is better to warn about unknown specifier instead of falling > back to the %p behavior. It will help people to understand what is > going wrong. They expect the IP address and not a pointer anyway > in this situation. > May be. If one sees a hashed value where IP address/device name/etc was meant to be then it's already a sign that something is wrong. Those WARN_ONCE that you have added make things simpler, I agree. A quick question, what happens on !CONFIG_BUG systems (where we have no_printk() WARN)? -ss
Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()
On (04/04/18 10:58), Petr Mladek wrote: > > Also it is better to warn about unknown specifier instead of falling > back to the %p behavior. It will help people to understand what is > going wrong. They expect the IP address and not a pointer anyway > in this situation. > May be. If one sees a hashed value where IP address/device name/etc was meant to be then it's already a sign that something is wrong. Those WARN_ONCE that you have added make things simpler, I agree. A quick question, what happens on !CONFIG_BUG systems (where we have no_printk() WARN)? -ss