RFCv2: debuginfod debian archive support

[Mark Wielaard]

Hi Frank,

On Fri, 2019-12-06 at 22:03 -0500, Frank Ch. Eigler wrote:
> @@ -851,7 +867,11 @@ handle_buildid_r_match (int64_t b_mtime,
> > >        return 0;
> > >      }
> > >  
> > > -  string popen_cmd = string("rpm2cpio " + shell_escape(b_source0));
> > > +  string archive_decoder = "/dev/null";
> > > +  for (auto&& arch : scan_archives)
> > > +    if (string_endswith(b_source0, arch.first))
> > > +      archive_decoder = arch.second;
> > > +  string popen_cmd = archive_decoder + " " + shell_escape(b_source0);
> > >    FILE* fp = popen (popen_cmd.c_str(), "r"); // "e" O_CLOEXEC?
> > >    if (fp == NULL)
> > >      throw libc_exception (errno, string("popen ") + popen_cmd);
> > 
> > This seems a lot of work for dealing with non-archives. 
> 
> We don't do this for non-archives.  This is in the path where an
> archive record already matched in the database.
> 
> > > +archive_classify (const string& rps, sqlite_ps& ps_upsert_buildids, sqlite_ps& ps_upsert_files,
> > >                sqlite_ps& ps_upsert_de, sqlite_ps& ps_upsert_sref, sqlite_ps& ps_upsert_sdef,
> > >                time_t mtime,
> > >                unsigned& fts_executable, unsigned& fts_debuginfo, unsigned& fts_sref, unsigned& fts_sdef,
> > >                bool& fts_sref_complete_p)
> > >  {
> > > -  string popen_cmd = string("rpm2cpio " + shell_escape(rps));
> > > +  string archive_decoder = "/dev/null";
> > > +  for (auto&& arch : scan_archives)
> > > +    if (string_endswith(rps, arch.first))
> > > +      archive_decoder = arch.second;
> > > +  string popen_cmd = archive_decoder + " " + shell_escape(rps);
> > >    FILE* fp = popen (popen_cmd.c_str(), "r"); // "e" O_CLOEXEC?
> > >    if (fp == NULL)
> > >      throw libc_exception (errno, string("popen ") + popen_cmd);
> > 
> > Likewise as above. Can we skip the whole popen dance if nothing
> > matches?
> 
> If you check out the caller, this part is not even called if
> the extension does not match.

I see, I missed that both functions are only called after first
checking the archive type. I think it might be helpful/clearer if both
methods would be called with the intended archive type then, also
because that might make it simpler to...

> > But it does feel like the errors, logs and metrics are a little
> > generic (e.g. "cannot select all format").
> 
> The way in which specializing the format errors could help if
> debuginfod were run against rpms that had a non-cpio payload, or debs
> that had a non-tar payload.  This means some sort of corruption, which
> contravenes our "trustworthy data" assumption -- or upstream policy
> change, which is nothing to worry about.
> 
> If you think separate metrics for .deb vs .rpm archives might be
> useful, can do.

If it isn't too much work then I do think it would be useful/clearer if
the logs/metrics reported debs and rpms separately.

Thanks,

Mark