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

[Jiri Olsa <olsajiri@gmail.com>]

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)

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 ?

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

jirka

>  }
>  
>  #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) || IS_ENABLED(CONFIG_VMCORE_INFO)
> -- 
> 2.43.0
> 
>