Messages in this thread | | | From | "Yang, Shunyong" <> | Subject | Re: [RFC PATCH] vsprintf: add flag ZEROPAD handling before crng is ready | Date | Sat, 27 Jan 2018 03:12:24 +0000 |
| |
Hi, Rasmus and Andy, Thanks for your feedback. I add some information below.
On Fri, 2018-01-26 at 10:43 +0100, Rasmus Villemoes wrote: > On 26 January 2018 at 10:17, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > +Rasmus > Thanks. > > > > > On Fri, 2018-01-26 at 15:39 +0800, Yang Shunyong wrote: > > > > > > Before crng is ready, output of "%p" composes of "(ptrval)" and > > > left padding spaces for alignment as no random address can be > > > generated. This seems a little strange sometimes. > > > For example, when irq domain names are built with "%p", the nodes > > > under /sys/kernel/debug/irq/domains like this, > > > > > > [root@y irq]# ls domains/ > > > default irqchip@ (ptrval)-2 > > > irqchip@ (ptrval)-4 \_SB_.TCS0.QIC1 \_SB_.TCS0.QIC3 > > > irqchip@ (ptrval) irqchip@ (ptrval)-3 > > > \_SB_.TCS0.QIC0 \_SB_.TCS0.QIC2 > > > > > > The name "irqchip@ (ptrval)-2" is not so readable in > > > console > > > output. > > Yes, this is not best output. > > > > > > > > This patch adds ZEROPAD handling in widen_string() and > > > move_right(). > > > When ZEROPAD is set in spec, it will use '0' for padding. If not > > > set, it will use ' '. > > > This patch also sets ZEROPAD in ptr_to_id() before crgn is ready. > Yew. > > > > > Have you added specific test cases to see what's going on for > > patterns > > like > > > > printf("%0s\n", " (my string)"); > [That's not really relevant, since we'll never have those (gcc says > "warning: '0' flag used with ‘%s’").] > > > > > > > > > @@ -1702,6 +1709,8 @@ static char *ptr_to_id(char *buf, char > > > *end, > > > void *ptr, struct printf_spec spec) > > > > > > if (unlikely(!have_filled_random_ptr_key)) { > > > spec.field_width = default_width; > > > + spec.flags |= ZEROPAD; > > > + > > > /* string length must be less than default_width */ > > > return string(buf, end, "(ptrval)", spec); > > > } > So why not just use a string literal with the right width to begin > with, e.g. =====(ptrval)===== or whatever manual padding left or > right > seems appropriate. Space-padding is not nice, but 0-padding isn't > much
Agree. 0-padding isn't much better. I prefers 'x' more as it means "unknown" and "don't care" sometimes. For "(ptrval)", shall we change to something indicating the output is hidden? Such as following candidate, [HIDDEN] [HASHED]
Then, on a 32-bit system, the output is "[HIDDEN]". On a 64-bit system, it is "[xxxxHIDDENxxxx]" or "[====HIDDEN====]". "[...]" is offen used in dmesg to represent address range and more readable in this case. But I am not sure whether there are other issues compare to "(...)".
> better. That way you only affect the uncommon case of %p before > have_filled_random_ptr_key instead of adding a few instructions to > all > %s users. >
Agree. My original idea was to reuse existing code. Exactly as you said, this is the uncommon case, change should be minimized.
> While at it, it may be worth looking into whether the irqdomain > output > actually needs the @%p thing or if one could improve that instead. >
I am inclined to make this patch a small enhancement to Tobin's "%p" series. As, 1. I am not sure whether someone may use "%p" in early stage unintentionally in the future. 2. In some race condition or different kernel configuration, is there possibility some code with "%p" run before crng is ready on one machine, but after on another? 3. I noticed other outputs in dmesg have the same issue (I deleted the timestamp to make log in one line in the mail),
Virtual kernel memory layout: modules : 0xffff000000000000 - 0xffff000008000000 ( 128 MB) vmalloc : 0xffff000008000000 - 0xffff7dffbfff0000 (129022 GB) .text : 0x (ptrval) - 0x (ptrval) ( 9600 KB) .rodata : 0x (ptrval) - 0x (ptrval) ( 4544 KB)
...
Remapping and enabling EFI services. EFI remap 0x0000000000200000 => (ptrval) EFI remap 0x0000000003080000 => (ptrval) EFI remap 0x0000000006300000 => (ptrval) ...
Thanks Shunyong
| |