This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: rfc/patch: debuginfod client $DEBUGINFOD_PROGRESS env var
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