On Thu, Jul 25, 2024 at 5:03 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Wed, Jul 24, 2024 at 03:52:01PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > +static int freader_get_page(struct freader *r, u64 file_off)
> > +{
> > + pgoff_t pg_off = file_off >> PAGE_SHIFT;
> > +
> > + freader_put_page(r);
> > +
> > + r->page = find_get_page(r->mapping, pg_off);
> > + if (!r->page)
> > + return -EFAULT; /* page not mapped */
> > +
> > + r->page_addr = kmap_local_page(r->page);
> > + r->file_off = file_off & PAGE_MASK;
> > +
> > + return 0;
> > +}
> > +
> > +static const void *freader_fetch(struct freader *r, u64 file_off, size_t sz)
> > +{
> > + int err;
> > +
> > + /* provided internal temporary buffer should be sized correctly */
> > + if (WARN_ON(r->buf && sz > r->buf_sz)) {
> > + r->err = -E2BIG;
> > + return NULL;
> > + }
>
> what's the benefit of having err, would it be easier just to return
> error pointer like ERR_PTR(-E2BIG)
>
There are many calls into freader_fetch() and I didn't want to have a
very distracting
p = freader_fetch(...)
if (IS_ERR(p)) {
err = PTR_ERR(p);
...
}
pattern everywhere
> SNIP
>
> > +static void freader_cleanup(struct freader *r)
> > +{
> > + freader_put_page(r);
> > +}
> > +
> > /*
> > * Parse build id from the note segment. This logic can be shared between
> > * 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are
> > * identical.
> > */
> > -static int parse_build_id_buf(unsigned char *build_id,
> > - __u32 *size,
> > - const void *note_start,
> > - Elf32_Word note_size)
> > +static int parse_build_id_buf(struct freader *r,
> > + unsigned char *build_id, __u32 *size,
> > + u64 note_offs, Elf32_Word note_size)
> > {
> > - Elf32_Word note_offs = 0, new_offs;
> > + const char note_name[] = "GNU";
>
> could be static ?
could be, but why? it's a 4 byte value, compiler might as well
optimize any sort of note_name[i] access into known constants
>
> SNIP
>
> > int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size)
> > {
> > - return parse_build_id_buf(build_id, NULL, buf, buf_size);
> > + struct freader r;
> > +
> > + freader_init_from_mem(&r, buf, buf_size);
> > +
> > + return parse_build_id_buf(&r, build_id, NULL, 0, buf_size);
>
> could use a coment in here why freader_cleanup is not needed
>
probably better to just include freader_cleanup() call, just in case?
> jirka
>
> > }
> >
> > #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) || IS_ENABLED(CONFIG_VMCORE_INFO)
> > --
> > 2.43.0
> >
> >
