From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 762E83858D28 for ; Tue, 2 May 2023 16:07:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 762E83858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683043665; 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: references:references; bh=Y0Yo5s/a8QEsi0PCes1PviE5RkLov8B2WyNkxF0uYe4=; b=drOAuvRatBffFwTtqiVdnVGKtU13xf+nh+BVb0cixVMaVDxLZVe46FNOcG9xT7LdBuCfnq i+aSaZHeOjNweHWvr+H6D11YgGRQMXFuT4gcsPrtVqUU+W13tNkCrcy1BiL9EERs+yoI3y HSxdH1HiK5Rbn5smYxfMiOHEAuCIAuY= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-462-hnambUzvNUG54aAqSxxUUg-1; Tue, 02 May 2023 12:07:43 -0400 X-MC-Unique: hnambUzvNUG54aAqSxxUUg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6A43D382888D; Tue, 2 May 2023 16:07:43 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.2.16.10]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8E23BC15BAD; Tue, 2 May 2023 16:07:42 +0000 (UTC) From: Florian Weimer To: Adhemerval Zanella via Libc-alpha Cc: Adhemerval Zanella , Luca Boccassi Subject: Re: [PATCH v2 3/3] linux: Add pidfd_getpid References: <20230420142037.4063169-1-adhemerval.zanella@linaro.org> <20230420142037.4063169-4-adhemerval.zanella@linaro.org> Date: Tue, 02 May 2023 18:07:35 +0200 Message-ID: <87r0rysomw.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: * Adhemerval Zanella via Libc-alpha: > This interface allows to obtain the associated pid ID from the > process file descriptor. It is done by parsing the procps fdinfo > information. Its prototype is: > > pid_t pidfd_getpid (int fd) > > It returns the associated pid or -1 in case of an error and set the > errno accordingly. The possible errno values are the smae from > open, read, and close (used on procps parsing), along with: > > - EINVAL if the FP is negative (similar to fexecve). > > - EBADF if the FD does not have a PID associated of if the fdinfo > fields contains a value larger than pid_t. > > - EREMOTE if the PID is in a separate namespace. > > - ESRCH if the process is already terminated. Could you add a manual entry for this? The documentation for EREMOTE (also in the comment in the code) should probably say that this is returned if there is no PID to denote the process in the current namespace. I assume that the PID is available from an outer namespace (even across PID namespace boundaries), but not necessarily in the other direction. > diff --git a/sysdeps/unix/sysv/linux/pidfd_getpid.c b/sysdeps/unix/sysv/linux/pidfd_getpid.c > new file mode 100644 > index 0000000000..d0c7987791 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/pidfd_getpid.c > @@ -0,0 +1,94 @@ > +static bool > +parse_fdinfo (const char *l, void *arg) > +{ > + enum { fieldlen = sizeof ("Pid:") - 1 }; > + if (strncmp (l, "Pid:", fieldlen) != 0) > + return true; > + > + l += fieldlen; > + > + char *endp; > + long int n = strtol (l, &endp, 10); > + if (l == endp || n > INT_MAX) > + return true; Should this use strtoul? Otherwise I'm not sure the overflow check will work on 32-bit. Rest of the implementation looks okay, but the kernel should really provide an ioctl or similar for this. > diff --git a/sysdeps/unix/sysv/linux/procutils.c b/sysdeps/unix/sysv/linux/procutils.c > new file mode 100644 > index 0000000000..4909afeae1 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/procutils.c > @@ -0,0 +1,99 @@ > +/* Utilities functions to read/parse Linux procfs and sysfs. > +int > +procutils_read_file (const char *filename, procutils_closure_t closure, > + void *arg) > +{ > + enum { buffer_size = 1024 }; > + char buffer[buffer_size]; > + char *buffer_end = buffer + buffer_size; > + char *cp = buffer_end; > + char *re = buffer_end; > + > + int fd = __open64_nocancel (filename, O_RDONLY | O_CLOEXEC); > + if (fd == -1) > + return -1; > + > + char *l; > + while ((l = next_line (fd, buffer, &cp, &re, buffer_end)) != NULL) > + if (!closure (l, arg)) > + break; > + > + __close_nocancel_nostatus (fd); > + > + return 0; > +} Incomplete error reporting? Any read failure doesn't make it to the caller. The callback interface will be easier to use if you have it return int and return the callback return value if it is non-zero, I think. > diff --git a/sysdeps/unix/sysv/linux/tst-pidfd.c b/sysdeps/unix/sysv/linux/tst-pidfd.c > index 64d8a2ef40..2a73328ef1 100644 > --- a/sysdeps/unix/sysv/linux/tst-pidfd.c > +++ b/sysdeps/unix/sysv/linux/tst-pidfd.c > @@ -18,6 +18,7 @@ It would be nice to have a check for the EREMOTE case. Thanks, Florian