From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x231.google.com (mail-oi1-x231.google.com [IPv6:2607:f8b0:4864:20::231]) by sourceware.org (Postfix) with ESMTPS id B307C3858D20 for ; Fri, 11 Aug 2023 15:31:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B307C3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oi1-x231.google.com with SMTP id 5614622812f47-3a3373211a1so1712094b6e.0 for ; Fri, 11 Aug 2023 08:31:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1691767884; x=1692372684; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=rhgjqghFbgBIbNO8pgymRt+rEwjmu8YxSwGrE4oNTKg=; b=q7//+QTBppXa7D/Y7APEkxDgxkNmshnbPiy+gdjc8pelKWG8MQCFep7wktBksWE81h n+Ixz/SQV6SWknmb53ogdyDrNXgvWJcrBN10G2omRxtHAGcSa7m4lVVv0008fzzNX5GT DSuPen3nGEHPOMlbabOPsti2gFcOAHXLH/WGNaWepfUfrTQceV4D39An1LQGdiQpp0N1 SNw+4DQtFRN1z02YuUXJOlYlM6j9HX2nqsijo7cLZmaPgopvNMiMDMmNV5LLSr9ySoRu fvZlpzRkh/zvkUl/hU3MZ6u6eLQYiXZirT5uevp9Ilp7k2b3paX78rZzozj2veiheIRM jvxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691767884; x=1692372684; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=rhgjqghFbgBIbNO8pgymRt+rEwjmu8YxSwGrE4oNTKg=; b=MSrifcpOLl5VKTDtZ/GPzKv7MDt4u8f74igdkG8SdP7nlWGePTGDnjXOkk9TxlGUt7 0l3zR/qkdFt8QIS33IYtTx4dmqO3DcgHsABq2vgiYQ+BfVnZMv8xNQcAbz+nI6DdOxvM ZH+M/gb56Cvt0ZOBybH/atOzzH0EflCk5JdJu5cRYAi+IPakL8564iisezYUfunbLEnX 3FMAtSCmQBT+kFhKGJ5E9WWMwV2T+CUzKlWSFk0O66Stjt554CU3ES6x42foJxa4vb2B McV4SyQ3dG1eRSAeQp7+GgDqInQRg0ydFDBlVNMQi/GdSmdci3TNW6F4ixECKHCvwalR 6KXg== X-Gm-Message-State: AOJu0YwcIRbm63TIfm7QX5nE2K+wChZOV7F8mvSMGaPuVx7DowB06g+O 96IFaSixLT/BiVO27SV/n8sNlURFo8h/SVojV9j0fw== X-Google-Smtp-Source: AGHT+IFhNSvKr0ECJcFkT21MKy1eogTElj4esDSjn6+Ofuzj6gzH6ydmplvfIWQOH3S1VIA/lAzmKA== X-Received: by 2002:a05:6808:341:b0:3a1:dfa0:7e18 with SMTP id j1-20020a056808034100b003a1dfa07e18mr2601065oie.25.1691767883943; Fri, 11 Aug 2023 08:31:23 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c2:365b:31b6:a2f1:3127:aa71? ([2804:1b3:a7c2:365b:31b6:a2f1:3127:aa71]) by smtp.gmail.com with ESMTPSA id a24-20020a05680804d800b003a5b027ccb2sm1810542oie.38.2023.08.11.08.31.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Aug 2023 08:31:23 -0700 (PDT) Message-ID: <3f04f730-5dcd-aa97-4bf1-34b1c9785c7b@linaro.org> Date: Fri, 11 Aug 2023 12:31:20 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.14.0 Subject: Re: [PATCH v7 4/8] linux: Add posix_spawnattr_{get, set}cgroup_np (BZ 26731) Content-Language: en-US To: Florian Weimer , Adhemerval Zanella via Libc-alpha References: <20230803163558.991832-1-adhemerval.zanella@linaro.org> <20230803163558.991832-5-adhemerval.zanella@linaro.org> <87y1ihvomc.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <87y1ihvomc.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP 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: On 11/08/23 07:51, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> These function allow to posix_spawn and posix_spawnp to use >> CLONE_INTO_CGROUP with clone3, allowing the child process to >> be created in a different version 2 cgroup. These are GNU >> extensions that are available only for Linux, and also only >> for the architectures that implement clone3 wrapper >> (HAVE_CLONE3_WRAPPER). >> >> To create a process on a different cgroupv2, one can use the: >> >> posix_spawnattr_t attr; >> posix_spawnattr_init (&attr); >> posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETCGROUP); >> posix_spawnattr_setcgroup_np (&attr, cgroup); >> posix_spawn (...) > > Why are both POSIX_SPAWN_SETCGROUP and posix_spawnattr_setcgroup_np > needed? Couldn't the latter imply the former? Besides being orthogonal with the other standard options, it allows the called to just set/reset a flag in a posix_spawnattr_t to enable disable the options instead of create/destroy a new attribute for each posix_spawn call. > >> There is no fallback is either clone3 does not support the flag >> or if the architecture does not provide the clone3 wrapper, in >> this case posix_spawn returns ENOTSUP. > > I think this really should be added to the manual, mayb > > It's also not clear to me how you would probe for support properly. > The spawn operation might fail for other reasons. > > I wonder if we have to probe as part of the Some comments seems to be truncated. For probing, posix_spawnattr_setflags fails with invalid flags, so trying to use POSIX_SPAWN_SETCGROUP if is is not support should return EINVAL. About the manual, I can add something but since we do not any sort of posix_spawn it would require to add a lot of stubs. > >> diff --git a/sysdeps/unix/sysv/linux/bits/spawn_ext.h b/sysdeps/unix/sysv/linux/bits/spawn_ext.h >> new file mode 100644 >> index 0000000000..3bc10ab477 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/bits/spawn_ext.h > >> +/* Get the cgroupsv2 the attribute structure. */ >> +extern int posix_spawnattr_getcgroup_np (const posix_spawnattr_t * >> + __restrict __attr, >> + int *__cgroup) >> + __THROW __nonnull ((1, 2)); >> + >> +/* Store scheduling parameters in the attribute structure. */ >> +extern int posix_spawnattr_setcgroup_np (posix_spawnattr_t *__restrict __attr, >> + int __cgroup) >> + __THROW __nonnull ((1)); > > Second comment seems wrong. Indeed, and there is no need of __restrict here. Also on posix_spawnattr_getcgroup_np it should have a __restrict for the cgroup argument. > >> diff --git a/sysdeps/unix/sysv/linux/tst-spawn-cgroup.c b/sysdeps/unix/sysv/linux/tst-spawn-cgroup.c >> new file mode 100644 >> index 0000000000..6dba30ab29 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-spawn-cgroup.c >> @@ -0,0 +1,216 @@ >> +/* Tests for posix_spawn cgroup extension. > >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ > > Should be “https://”. Ack. > >> +#define F_TYPE_EQUAL(a, b) (a == (typeof(a)) b) > > Missing space after “type”. Ack. > >> +static char * >> +get_cgroup (void) >> +{ >> + FILE *f = fopen ("/proc/self/cgroup", "re"); >> + if (f == NULL) >> + FAIL_UNSUPPORTED ("no cgroup defined for the process"); > > Maybe add %m here. Ack. > >> +/* Called on process re-execution. */ >> +_Noreturn static void >> +handle_restart (int argc, char *argv[]) >> +{ >> + assert (argc == 1); >> + char *newcgroup = argv[0]; >> + >> + char *current_cgroup = get_cgroup (); >> + TEST_VERIFY_EXIT (current_cgroup != NULL); >> + TEST_COMPARE_STRING (newcgroup, current_cgroup); >> + exit (EXIT_SUCCESS); >> +} > > I think the exit (EXIT_SUCCESS) masks failures because after execve, the > shared mapping with failure status does not exist. The TEST_VERIFY_EXIT should trigger the waitid checks, but you are right for TEST_COMPARE_STRING. I removed the exit to let the support/test-driver.c return the expected exit code. > >> +static int >> +create_new_cgroup (char **newcgroup) >> +{ >> + struct statfs fs; >> + if (statfs (CGROUPFS, &fs) < 0) >> + { >> + if (errno == ENOENT) >> + FAIL_UNSUPPORTED ("not cgroupv2 mount found"); >> + FAIL_EXIT1 ("statfs (%s): %m\n", CGROUPFS); > > “no[] cgroupv2 found?” Ack. > >> + } >> + >> + if (!F_TYPE_EQUAL (fs.f_type, CGROUP2_SUPER_MAGIC)) >> + FAIL_UNSUPPORTED ("%s is not a cgroupv2", CGROUPFS); > > This could print fs.f_type. Ack.