[Bug,debuginfod/27982] added DEBUGINFOD_MAXSIZE and DEBUGINFOD_MAXTIME

  Hi Noah,

On Mon, 2021-07-26 at 14:56 -0400, Noah Sanci via Elfutils-devel wrote:
> Please find the attached patch for pr 27982. DEBUGINFOD_MAXSIZE and
> MAXTIME were added in this patch to allow users to use more
> constraints when downloading debuginfo.

This looks good. Some high level comments (because I don't actually
know much about http requests).

Why have MAXTIME default to LONG_MAX? Which is long, but different on
different arches 32/64bit. If MAXSIZE == 0 means infinite, why not make
 MAXTIME=0 the same for consistency?

An alternative of passing around the X-DEBUGINFOD-MAXSIZE header is
doing it all at the client side if we supported HEAD. See PR27277. Did
you consider that route?

The bug suggests to also check the Content-Length header on reciept (in
case the server didn't support or respect the X-DEBUGINFOD-MAXSIZE
header. Did you try to implement that?

I believe various error handling goto out1 should be goto out2 (after
your duplicate urls patch). Could you double check that?

I had to make this little tweak to make the testcase pass on my setup
(because PWD contains symlinks):


Cheers,

Mark
  

Hi -

> [...]  An alternative of passing around the X-DEBUGINFOD-MAXSIZE
> header is doing it all at the client side if we supported HEAD. See
> PR27277. Did you consider that route? [...]

That would require -two- http round-trips (one HEAD, one actual GET).
This way, the requested limit goes out in the initial GET request.

- FChE
  

Hello,

> Why have MAXTIME default to LONG_MAX? Which is long, but different on
> different arches 32/64bit. If MAXSIZE == 0 means infinite, why not make
>  MAXTIME=0 the same for consistency?

Fixed.

> The bug suggests to also check the Content-Length header on reciept (in
> case the server didn't support or respect the X-DEBUGINFOD-MAXSIZE
> header. Did you try to implement that?

Fixed.

> I believe various error handling goto out1 should be goto out2 (after
> your duplicate urls patch). Could you double check that?

Fixed.

> diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> index feec5ddf..8ed7743d 100755
> --- a/tests/run-debuginfod-find.sh
> +++ b/tests/run-debuginfod-find.sh
> @@ -187,7 +187,7 @@ tempfiles find-vlog$PORT1
>  # wait for the server to fail the same number of times the query is retried.
>  wait_ready $PORT1 'http_responses_after_you_milliseconds_count{code="406"}' 1
>  # quickly ensure all reporting is functional
> -grep 'serving file '${PWD}'/F/p+r%o\$g.debug' vlog$PORT1
> +grep 'serving file '$(realpath ${PWD})'/F/p+r%o\$g.debug' vlog$PORT1
>  grep 'File too large' vlog$PORT1
>  grep 'using max size 1B' find-vlog$PORT1
>  if [ -f ${DEBUGINFOD_CACHE_PATH}/${BUILDID} ]; then
Fixed.

Find all the fixes in the attached patch.

Thanks,
Noah
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debuginfod-PR27983-ignore-duplicate-urls.patch
Type: text/x-patch
Size: 9879 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20210729/91cc1831/attachment-0001.bin>
  

Hi Noah,

On Thu, 2021-07-29 at 16:29 -0400, Noah Sanci wrote:
> Why have MAXTIME default to LONG_MAX? Which is long, but different
> > on
> > different arches 32/64bit. If MAXSIZE == 0 means infinite, why not make
> >  MAXTIME=0 the same for consistency?
> 
> Fixed.
> 
> > The bug suggests to also check the Content-Length header on reciept (in
> > case the server didn't support or respect the X-DEBUGINFOD-MAXSIZE
> > header. Did you try to implement that?
> 
> Fixed.
> 
> > I believe various error handling goto out1 should be goto out2 (after
> > your duplicate urls patch). Could you double check that?
> 
> Fixed.
> 
> > diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> > index feec5ddf..8ed7743d 100755
> > --- a/tests/run-debuginfod-find.sh
> > +++ b/tests/run-debuginfod-find.sh
> > @@ -187,7 +187,7 @@ tempfiles find-vlog$PORT1
> >  # wait for the server to fail the same number of times the query is retried.
> >  wait_ready $PORT1 'http_responses_after_you_milliseconds_count{code="406"}' 1
> >  # quickly ensure all reporting is functional
> > -grep 'serving file '${PWD}'/F/p+r%o\$g.debug' vlog$PORT1
> > +grep 'serving file '$(realpath ${PWD})'/F/p+r%o\$g.debug' vlog$PORT1
> >  grep 'File too large' vlog$PORT1
> >  grep 'using max size 1B' find-vlog$PORT1
> >  if [ -f ${DEBUGINFOD_CACHE_PATH}/${BUILDID} ]; then
> 
> Fixed.

Great. Thanks. Although you don't just have to do things the way I like
them. Feel free to push back and tell me you feel the solution you
chose is better than what I am suggesting. It really is a conversation.

> Find all the fixes in the attached patch.

Looks like you attached the wrong patch (url-duplicate)

Cheers,

Mark
  

Hello,

Here is the real patch :).

Thanks,
Noah

On Fri, Jul 30, 2021 at 7:11 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi Noah,
>
> On Thu, 2021-07-29 at 16:29 -0400, Noah Sanci wrote:
> > Why have MAXTIME default to LONG_MAX? Which is long, but different
> > > on
> > > different arches 32/64bit. If MAXSIZE == 0 means infinite, why not make
> > >  MAXTIME=0 the same for consistency?
> >
> > Fixed.
> >
> > > The bug suggests to also check the Content-Length header on reciept (in
> > > case the server didn't support or respect the X-DEBUGINFOD-MAXSIZE
> > > header. Did you try to implement that?
> >
> > Fixed.
> >
> > > I believe various error handling goto out1 should be goto out2 (after
> > > your duplicate urls patch). Could you double check that?
> >
> > Fixed.
> >
> > > diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> > > index feec5ddf..8ed7743d 100755
> > > --- a/tests/run-debuginfod-find.sh
> > > +++ b/tests/run-debuginfod-find.sh
> > > @@ -187,7 +187,7 @@ tempfiles find-vlog$PORT1
> > >  # wait for the server to fail the same number of times the query is retried.
> > >  wait_ready $PORT1 'http_responses_after_you_milliseconds_count{code="406"}' 1
> > >  # quickly ensure all reporting is functional
> > > -grep 'serving file '${PWD}'/F/p+r%o\$g.debug' vlog$PORT1
> > > +grep 'serving file '$(realpath ${PWD})'/F/p+r%o\$g.debug' vlog$PORT1
> > >  grep 'File too large' vlog$PORT1
> > >  grep 'using max size 1B' find-vlog$PORT1
> > >  if [ -f ${DEBUGINFOD_CACHE_PATH}/${BUILDID} ]; then
> >
> > Fixed.
>
> Great. Thanks. Although you don't just have to do things the way I like
> them. Feel free to push back and tell me you feel the solution you
> chose is better than what I am suggesting. It really is a conversation.
>
> > Find all the fixes in the attached patch.
>
> Looks like you attached the wrong patch (url-duplicate)
>
> Cheers,
>
> Mark
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debuginfod-PR27982-added-DEBUGINFOD_MAXSIZE-and-DEBU.patch
Type: text/x-patch
Size: 13148 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20210730/c4e8f861/attachment-0001.bin>
  

On Fri, 2021-07-30 at 09:39 -0400, Noah Sanci wrote:
> Here is the real patch :).

Looks good. Pushed.

Thanks,

Mark