Re: [PATCH v2 bpf-next 01/10] lib/buildid: add single page-based file reader abstraction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> >
> >





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux