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
