Re: rfc/patch: debuginfod client $DEBUGINFOD_PROGRESS env var

["Frank Ch. Eigler" <fche@redhat.com>]

Hi -

> >     debuginfod: usability tweaks, incl. $DEBUGINFOD_PROGRESS client
> > support
> >     
> >     This facility allows a default progress-printing function
> >     to be installed if the given environment variable is set.
> 
> I like this idea but have a bit of a security concern about the ability
> to set it to any file. If someone manages to set it then they can
> overwrite anything a process using the library can write to. 

That's not that serious a category of concern.  Environment variables
are not under control of untrusted agents.  FWIW, $DEBUGINFOD_CACHE is
considerably more dangerous in that regard (cache cleaning!).

> I rather it would just use stderr always when set since it is only
> meant as a human debugging device.

That default makes sense.  Making it configurable gives a way for a
larger app to still capture output (by having the client direct it to
a pipe file descriptor or something), but I'll just hardcode for now
STDERR_FILENO if the env var is set.


> >     Some larger usage experience (systemtap/kernels) indicates
> >     the default timeout is too short, so bumped it to 30s.

> My first thought is that we might need two timeouts. The original 5s
> timeout is a good inactivity timeout [...] But for larger data we
> might want a timeout for the total download time, maybe 30 seconds
> is too low for that. Given how large kernel debuginfo is it might
> take several minutes.

OK, redrafting $DEBUGINFOD_TIMEOUT as a two-part variable:

$DEBUGINFOD_TIMEOUT=x,y
 x - connection timeout, default 5
 y - optional, overall timeout, default unlimited

> [...]
> This seems to mean that if there is an error then the progress function
> is never called, is that intended? Also for CURLM_CALL_MULTI_PERFORM we
> continue/skip the progress function.

If we have an error outcome, there is no more progress to report, so
that should be OK.


> > +  close (fd); /* before the rmdir, otherwise it'll fail */
> >    (void) rmdir (target_cache_dir); /* nop if not empty */
> >    free(data);
> > -  close (fd);
> 
> This looks good. Just surprising. I assumed unlinking the file was
> enough.

Yeah, I had to see that for myself to believe it.


> The \r \n trick is nice.
> Just always using stderr would simplify this a bit.

OK.

> Also note that you can use dprintf to printf to a file descriptor. 

OK.


- FChE