Re: [PATCH v5 13/13] kdump: Use vmlinux_build_id to simplify

[Baoquan He]

On 05/07/21 at 11:54am, Baoquan He wrote:
> On 04/29/21 at 01:14am, Stephen Boyd wrote:
> > Quoting Baoquan He (2021-04-28 03:13:55)
> > > On 04/20/21 at 02:50pm, Stephen Boyd wrote:
> > > > We can use the vmlinux_build_id array here now instead of open coding
> > > > it. This mostly consolidates code.
> > > >
> > > > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > > > Cc: Jessica Yu <jeyu@xxxxxxxxxx>
> > > > Cc: Evan Green <evgreen@xxxxxxxxxxxx>
> > > > Cc: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> > > > Cc: Dave Young <dyoung@xxxxxxxxxx>
> > > > Cc: Baoquan He <bhe@xxxxxxxxxx>
> > > > Cc: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > > > Cc: <kexec@xxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > > > ---
> > > >  include/linux/crash_core.h | 12 ++++-----
> > > >  kernel/crash_core.c        | 50 ++------------------------------------
> > > >  2 files changed, 8 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > > > index 206bde8308b2..de62a722431e 100644
> > > > --- a/include/linux/crash_core.h
> > > > +++ b/include/linux/crash_core.h
> > > > @@ -38,8 +38,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
> > > >
> > > >  #define VMCOREINFO_OSRELEASE(value) \
> > > >       vmcoreinfo_append_str("OSRELEASE=%s\n", value)
> > > > -#define VMCOREINFO_BUILD_ID(value) \
> > > > -     vmcoreinfo_append_str("BUILD-ID=%s\n", value)
> > > > +#define VMCOREINFO_BUILD_ID()                                                \
> > > > +     ({                                                              \
> > > > +             static_assert(sizeof(vmlinux_build_id) == 20);          \
> > > > +             vmcoreinfo_append_str("BUILD-ID=%20phN\n", vmlinux_build_id); \
> > >
> > > Since there has been static_assert at above, can we remove the magic
> > > number '20'?
> > >
> > > And I checked format_decode(), didn't find which type corresponds to
> > > 'N', could you tell?
> > 
> > It is documented in Documentation/core-api/printk-formats.rst (see "Raw
> > buffer as a hex string") where the '20' replaces the '*' to specify a
> > fixed size buffer to format. We could pass the 20 as an argument instead
> > of encoding it in the format, but then it would still be there twice. We
> > could use a #define but then it would have to be passed as an argument.
> 
> Thanks for telling, I can see 'phN' is used in many places, but haven't
> figured out how it's parsed, e.g in format_decode() it's not related.
> Anyway, I may need dig further or search to get it.
> 
> > 
> > I don't know of a good way to enforce in printk that the buffer is the
> > size that matches the format size. Maybe it shouldn't be an option to
> > have a size in the format string if it's a problem to put the size of
> > the buffer in there.
> 
> Oh, I meant that we may not need to enforce that because the size of
> vmlinux_build_id can be guaranteed, always 20, we do not need to specify
> the '20'? Afaik, we usually specify the format size when the string size
> is variable, and we want to add '0' to make the width of printing is
                                  ~'space' I meant
> fixed. Anyway, this dones't matter much.
> 
> Acked-by: Baoquan He <bhe@xxxxxxxxxx>
> 
> Thanks
> Baoquan
> 
> > 
> > >
> > > Other than these, this patch looks good to me, thanks for the effort.
> > >
> > 
> > Thanks.
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/kexec
> > 
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/kexec
>