From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118432 invoked by alias); 15 Nov 2019 21:00:58 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 118373 invoked by uid 89); 15 Nov 2019 21:00:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.1 spammy=insist, damage, scanner, it'll X-Spam-Status: No, score=-3.3 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 15 Nov 2019 21:00:55 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1573851653; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xvUJACR587WegEr9nTWgeBSlkz3brQdbzwWy5RjClng=; b=QMa8VZ3F1f4ou9TdK7Sgac7GvQIMRaQdGCeSDIuCpUxja44PFe4yBCPLEATXNpRR/ZUVZM 2/dQ4bRgZqEVjXeIvIt7SxAxFM6pFAyemz7KvCGP4S9Yj2Lr0wSUFtLq2Iz6nc2CWyBIvJ mxf5kYQ/5dC/tFaloSgBWBxfSvCsUeA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-78-OP2VuACBMMqPPtC63eZ5bw-1; Fri, 15 Nov 2019 16:00:50 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7CB7A802690; Fri, 15 Nov 2019 21:00:49 +0000 (UTC) Received: from redhat.com (ovpn-116-19.phx2.redhat.com [10.3.116.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 48AAC608C9; Fri, 15 Nov 2019 21:00:49 +0000 (UTC) Received: from fche by redhat.com with local (Exim 4.92) (envelope-from ) id 1iVihz-00052B-Lu; Fri, 15 Nov 2019 16:00:47 -0500 Date: Fri, 15 Nov 2019 21:00:00 -0000 From: "Frank Ch. Eigler" To: Mark Wielaard Cc: elfutils-devel@sourceware.org, amerey@redhat.com Subject: Re: patch 2/2 debuginfod server etc. Message-ID: <20191115210047.GD15272@redhat.com> References: <20191028190438.GC14349@redhat.com> <20191028190602.GD14349@redhat.com> <20191028190726.GE14349@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.12.0 (2019-05-25) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-MC-Unique: OP2VuACBMMqPPtC63eZ5bw-1 X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline X-IsSubscribed: yes X-SW-Source: 2019-q4/txt/msg00151.txt.bz2 Hi - > > +// Roll this identifier for every sqlite schema incompatiblity. > > +#define BUILDIDS "buildids9" > > + > > +#if SQLITE_VERSION_NUMBER >=3D 3008000 > > +#define WITHOUT_ROWID "without rowid" > > +#else > > +#define WITHOUT_ROWID "" > > +#endif > > + > > +static const char DEBUGINFOD_SQLITE_DDL[] =3D > > + "pragma foreign_keys =3D on;\n" > > + "pragma synchronous =3D 0;\n" // disable fsync()s - this cache is di= sposa> [...] > Is there any way these sql DDL statements can be put somewhere else? Not an easy way. Some of the content is parametrized based on sqlite version; some of it on the schema-version literal BUILDIDS. > > +// Print a standard timestamp. > > +static ostream& > > +timestamp (ostream &o) > > + char *now2 =3D ctime (&now); > I think you want to use ctime_r with a stack allocated char[26]. > Also does the now2[19] always work? Isn't it better to strchr for the > '\n'. Or maybe just use strftime "%c"? Good idea. > [...] > Note that newer glibc actually define a function called gettid: > https://sourceware.org/bugzilla/show_bug.cgi?id=3D6399 > So you might want to rename it to something that doesn't accidentally > clashes. OK. > > +static string > > +conninfo (struct MHD_Connection * conn) > > +{ > > + char hostname[128]; > > + char servname[128]; >=20 > boo, hardcode name sizes. But ok. Yeah, that's the nature of the API though. Will bump up those sizes a bit. > > + string popen_cmd =3D string("/usr/bin/rpm2cpio " + shell_escape(b_so= urce0)); >=20 > Why the hardcoded path? > Could you check at startup if rpm2cpio is in the PATH? Hm, since this is run under popen(), so it'll do $PATH searching for us. Checking whether it is present at runtime ... hmmm ugh ... how about an autoconf test instead? The worst thing that happens with the current code is that on a non-rpm system, if it does find .rpm files, the code will print errors and otherwise ignore RPMs. > > + // extract this file to a temporary file > > + char tmppath[PATH_MAX] =3D "/tmp/debuginfod.XXXXXX"; // XXX: $TM= P_DIR etc. >=20 > Some other code uses: > const char *tmpdir =3D getenv ("TMPDIR") ?: P_tmpdir; > static const char suffix[] =3D "/debuginfod.XXXXXX"; > Also PATH_MAX? OK -- and yeah if we getenv() we might need PATH_MAX. Will try asprintf() here and the client instead. > > + rc =3D archive_read_data_into_fd (a, fd); > > + struct MHD_Response* r =3D MHD_create_response_from_fd (archive_= entry_size(e), fd); > > + if (r =3D=3D 0) > > + { > > + if (verbose) > > + obatched(clog) << "cannot create fd-response for " << b_so= urce0 << endl; > > + close(fd); > > + } >=20 > Should this break; ? Sure, continuing the loop just wastes time. > Also I prefer checking against NULL, it is slightly more obvious (0 > returns often means success). ... the C++ tradition is 0 for null pointers, but if you insist... > BTW. The usage of "R" or _r_ in the code is slightly confusing. I > would prefer it to just say RPM. Or isn't that what is meant? Changed in the diagnostics, for user consumption. In the code, I kind of like the short size that lets f and r (and near future d debs) look pretty parallel. > > +//////////////////////////////////////////////////////////////////////= // > > + > > + > > +// borrowed from src/nm.c get_local_names() >=20 > This is slightly misleading, most of the function is not from there. Added a clarifying adverb for this credit kudos. > > + Dwarf_Off offset =3D 0; > > + Dwarf_Off old_offset; > > + size_t hsize; > > + > > + while (dwarf_nextcu (dbg, old_offset =3D offset, &offset, &hsize, NU= LL, NULL, NULL) =3D=3D 0) >=20 > These days I would prefer dwarf_get_units (). It is slightly higher > level and immediately gives you the cudie and unit_type. Will look into that later. > > + { > > + Dwarf_Die cudie_mem; > > + Dwarf_Die *cudie =3D dwarf_offdie (dbg, old_offset + hsize, &cud= ie_mem); > > + > > + if (cudie =3D=3D NULL) > > + continue; > > + if (dwarf_tag (cudie) !=3D DW_TAG_compile_unit) > > + continue; > > + > > + const char *cuname =3D dwarf_diename(cudie) ?: "unknown"; > > + > > + Dwarf_Files *files; > > + size_t nfiles; > > + if (dwarf_getsrcfiles (cudie, &files, &nfiles) !=3D 0) > > + continue; >=20 > So you are really only interested in the file/line tables. > In that case you could also use dwarf_next_lines which iterates through > the debug line units directly, so you don't need to do the whole CU DIE > tree iteration yourself (and it handles CUless tables). Ditto. > > + string waldo; > > + if (hat[0] =3D=3D '/') // absolute > > + waldo =3D (string (hat)); > > + else // comp_dir relative > > + waldo =3D (string (comp_dir) + string("/") + string (hat)); >=20 > Do you have to think about/handle a comp_dir that ends with a / ? > Old debugedit truncated some strings by adding /// (to fill up the > spaces till the '\0'...) Yes, terrible :{ It should just work(tm) if the debugger uses the documented convention and preserves those extra slashes (just as if it preserved ../ and such). > > + // NB: we catch exceptions from elf_classify here too,= so that we can > > + // cache the corrupt-elf case (!executable_p && !debug= info_p) just below, > > + // just as if we had an EPERM error from open(2). > > +=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 > > + catch (const reportable_exception& e) > > + { > > + e.report(clog); > > + } >=20 > I think the comment is wrong since elf_classify seems to eat its own > reportable_exceptions (and those thrown from > dwarf_extract_source_paths) because it has its own try catch for > reportable_exceptions and doesn't rethrow them. Hm yeah, the comment describes an earlier version of code. Will just tweak the comment for now, but may be able to simplify the elf_classify() side later on, using one of those fancy deferred_dtor<> widgets. > > + sleep (1); > > + rescan_timer ++; > > + if (rescan_s) > > + rescan_timer %=3D rescan_s; > > + } > > +=20=20 > > + return 0; > > +} >=20 > Can we use something nicer than the hardcode sleep (1) ? Sort of ... doing it this way allows us to have up-to-date metrics (as per the following patch) as to how much longer to expect to wait for any given scanner thread to get going again ... and protects us from the case where only a single particular thread gets interrupted to serve a signal like SIGUSR*, and the others just sleep for the much larger rescan_s times, and miss responding quickly. > > + // XXX: _r_dalt >=20 > What? Earlier thoughts, nuked. > > + if (0) // XXX: if unsatisfied debugalt set is non-emp= ty ...: > > + break; >=20 > What? Another earlier thought, mooted by code just later. > > +static void > > +signal_handler (int /* sig */) > > +{ > > + interrupted ++; > > + > > + if (db) > > + sqlite3_interrupt (db); > > +=20=20 > > + // NB: don't do anything else in here > > +} >=20 > Nothing ever sets db to NULL after sqlite3_open. > The documentation of sqlite3_interrupt says: >=20 > "it is not safe to call this routine with a database connection that is > closed or might close before sqlite3_interrupt() returns" >=20 > So it seems this might cause trouble when this is called just > before/while sqlite3_close (db) is being called. Good catch, will clear db at close time. OTOH the damage from such a very late even SIGSEGV would be small indeed, as by then, all the databases are closed, threads shut down, etc. - FChE