From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22884 invoked by alias); 20 Mar 2015 19:12:29 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22874 invoked by uid 89); 20 Mar 2015 19:12:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 20 Mar 2015 19:12:27 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 9D9128E710 for ; Fri, 20 Mar 2015 19:12:26 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2KJCOwH018917; Fri, 20 Mar 2015 15:12:25 -0400 Message-ID: <550C7118.6080605@redhat.com> Date: Fri, 20 Mar 2015 19:12:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Sergio Durigan Junior , GDB Patches Subject: Re: [PATCH 1/2] Implement support for checking /proc/PID/coredump_filter References: <1426807358-18295-1-git-send-email-sergiodj@redhat.com> <1426807358-18295-2-git-send-email-sergiodj@redhat.com> In-Reply-To: <1426807358-18295-2-git-send-email-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00669.txt.bz2 Thanks, this looks good to me now, just a few minor nits. On 03/19/2015 11:22 PM, Sergio Durigan Junior wrote: > +/* Helper function to decode the "VmFlags" field in /proc/PID/smaps. > + > + This function was based on the documentation found on > + , on the Linux kernel. > + > + Linux kernels before commit > + 834f82e2aa9a8ede94b17b656329f850c1471514 do not have this field on > + smaps. */ Thanks for the version info. Ideally, what I'd like with these version notes is at any point in the time in the future being able to look at the code and easily identify whether we're working around an old enough kernel that we potentially can stop worrying about backwards compatibility with such kernel. If easy, could you also add the kernel version, like e.g., "$hash (x.y.z)"? > + if (init_regex_p == -1) > + { > + /* There was an error while compiling the regex'es above. In > + order to try to give some reliable information to the caller, > + we just try to find the string "(deleted)" in the filename. > + If we managed to find it, then we assume the mapping is > + anonymous. */ > + return strstr (filename, "(deleted)") != NULL; Might as well add the space before parens, and match tail end only, like e.g.,: const char deleted[] = " (deleted)"; size_t del_len = strlen (deleted); size_t filename_len = strlen (filename); return (filename_len >= del_len && strcmp (filename + filename_len - del_len, deleted) == 0); > + > +/* Return 0 if the memory mapping (which is related to FILTERFLAGS, V, > + MAYBE_PRIVATE_P, and MAPPING_ANONYMOUS_P) should not be dumped, or > + greater than 0 if it should. > + > + In a nutshell, this is the logic that we follow in order to decide > + if a mapping should be dumped or not. > + > + - If the mapping is associated to a file whose name ends with " > + (deleted)" Nit: it'd be good to avoid breaking " (deleted)" over two lines. > , or if the file is "/dev/zero", or if it is > + "/SYSV%08x" (shared memory), or if there is no file associated > + with it, or if the AnonHugePages: or the Anonymous: fields in the > + /proc/PID/smaps have contents, then GDB considers this mapping to > + be anonymous. Otherwise, GDB considers this mapping to be a > + file-backed mapping (because there will be a file associated with > + it). > + > + It is worth mentioning that, from all those checks described > + above, the most fragile is the one to see if the file name ends > + with " (deleted)". This does not necessarily mean that the > + mapping is anonymous, because the deleted file associated with > + the mapping may have been a hard link to another file, for > + example. The Linux kernel checks to see if "i_nlink == 0", but > + GDB cannot easily do this check. Therefore, we made a compromise Cannot do it easily, or cannot do it at all? > + here, and we assume that if the file name ends with " (deleted)", > + then the mapping is indeed anonymous. FWIW, this is something > + the Linux kernel could do better: expose this information in a > + more direct way. > + > + - If we see the flag "sh" in the "VmFlags:" field (in > + /proc/PID/smaps), then certainly the memory mapping is shared > + (VM_SHARED). If we have access to the VmFlags, and we don't see > + the "sh" there, then certainly the mapping is private. However, > + Linux kernels before commit > + 834f82e2aa9a8ede94b17b656329f850c1471514 do not have the > + "VmFlags:" field; in that case, we use another heuristic: if we > + see 'p' in the permission flags, then we assume that the mapping > + is private, even though the presence of the 's' flag there would > + mean VM_MAYSHARE, which means the mapping could still be private. > + This should work OK enough, however. */ > + > +static int > +dump_mapping_p (unsigned int filterflags, const struct smaps_vmflags *v, > + int maybe_private_p, int mapping_anon_p, const char *filename) > +{ > + /* Initially, we trust in what we received from outside. This value outside == the caller? or the kernel? The former, right? > + may not be very precise (i.e., it was probably gathered from the > + permission line in the /proc/PID/smaps list, which actually > + refers to VM_MAYSHARE, and not VM_SHARED), but it is what we have > + for now. */ "for now" refers to the flag being updated further down, or future kernel versions? > + int private_p = maybe_private_p; > + > + /* We always dump vDSO and vsyscall mappings. */ Suggest: /* We always dump vDSO and vsyscall mappings, because it's likely that there'll be no file to read the contents from at core load time. The kernel does the same. */ Thanks, Pedro Alves