From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id 9BF28A95D; Thu, 30 Jul 2026 00:40:32 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id DD663A940; Thu, 30 Jul 2026 00:40:29 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-0.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_PASS,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=4.0.1 Received: from mail-yw1-x1131.google.com (mail-yw1-x1131.google.com [IPv6:2607:f8b0:4864:20::1131]) by atuin.qyliss.net (Postfix) with ESMTPS id 48F73A8F3 for ; Thu, 30 Jul 2026 00:40:28 +0000 (UTC) Received: by mail-yw1-x1131.google.com with SMTP id 00721157ae682-80dc4a68e4aso20715087b3.0 for ; Wed, 29 Jul 2026 17:40:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1785372026; x=1785976826; darn=spectrum-os.org; h=content-type:autocrypt:in-reply-to:content-language:references:cc :to:subject:from:user-agent:mime-version:date:message-id:from:to:cc :subject:date:message-id:reply-to:content-type; bh=ycp8bvHjbAp0qNJCIYmBnvTwOLtpm7Tr0oCphiS0oqA=; b=SyyeBKpU3KtQGNDkDnNfbC1OJKiNxOtSMceCInPsXVcJpYvkFNpJQbyGf+UUHxt/8/ GNAPz1X6cLq/9lz+8vJqU/Apg1eDP+omzYwo7cbQuGgLZg6OJQamOhOAqvTRQ/DPHzb9 KZoQ80H1HbJnuoBgpiSLZi4iC3/9UdcnAaRk/ukOaa8kzXeqs22vgnjQc86SmlsqTkxq g7J2Drm82VX20/jGL+MMhZpcAJcGXdpBJP3CbdSCZvB7n7o9owR+nWA7rg/g8LnFG8xL 2jcizLSDU7FDK8R3Z5o44IfYoyGA6K4sd0Vc8Bj7YnmM9buHKE79KpuGbbKrLySorca8 Zq1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1785372026; x=1785976826; h=content-type:autocrypt:in-reply-to:content-language:references:cc :to:subject:from:user-agent:mime-version:date:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=ycp8bvHjbAp0qNJCIYmBnvTwOLtpm7Tr0oCphiS0oqA=; b=Ll7jy/NX8/gnMEp/84reICpOhCbJat4ZElHhL7zNbDQBT4hucsPv6NcTs+REnpzBzQ FV7HIZlTmgvbS27Xb4/SIK8byoJ+vQTmypv23EU9RZ3LCa8KHxBjdYhuINstWbf/1a/s WmvfXxyNsWUYN3IbMMfGMQB7UzYdhWXwXNig2Kb8krE2HgAoHgWHVXITXYjjB2KXMe0H uFJZavRGwUpb+jONEXfWKcV2XYK4dINpQ5oFXmvaXeXqe3ESU1732W/RTfEVhEA7SqQ7 8Z+CRvrjnzU8TvCAQnLqJDPk1Hb2+97l/WHv5CJK5ffBhPMM/uiD+H8aQCD47rL/ThOl /uSQ== X-Gm-Message-State: AOJu0YxXNC6r6k7vBb9TpbScKh3hx9sx21EMGjgPe3S0Jt2WcyGD0ZX0 i1vU+gBLmRtPMnC+4llk7+A8ULyehXsZ+vigSNqIGBB0w8yLG0WUx01wzDZPZA== X-Gm-Gg: AR+sD11VnAbtluZJ4o7QQv5A4y2YKgYzvW+JEpLheIceN3CPoW8PThNBHhQl9IrBgzC +Hjn/YsuX619XYn7iEn9aQA3yW190CJax4OUrVW3FeZuyPcgSjX+DCchwmPqCuGdiVksJrhjpNO hlFiKyVoUafdflDltySq1ZDqdtlbixSN093wmVXtkJ2rE8GuY7EeAE/Wny85/NsTP/t92l+zClO S8dJnI1tneXBxFde5WS5GhaZV37oR2w6/29NX/fu7Vd9l/KLMBjgqMG8JBtYcS0aPFueN7rjS62 T0z7gSh65ivCRMmlRyzo3eeR2xn29ke/FybzJaFOsyn2GkUEXYgF01q0bdKwmSLYHqCfzN+3lGy 0y8C5k89ql3f/2QswGgj4BZVEoR3jyV/n5MYCMxDb81kjSD4dr5G1/3cBWZSBVdpxEBzH63NGKV x7/YKTQDV2DVx+hPx5jaJbMVgOmQJXHbqX3iTtPPKREtLvwlX+gfe+B1KiIoXhmLM= X-Received: by 2002:a05:690c:4d87:b0:81e:f825:5840 with SMTP id 00721157ae682-81fb89b8302mr3299497b3.21.1785372026108; Wed, 29 Jul 2026 17:40:26 -0700 (PDT) Received: from [10.138.10.6] ([185.98.168.14]) by smtp.gmail.com with ESMTPSA id 00721157ae682-81fb8a89719sm1383517b3.27.2026.07.29.17.40.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 29 Jul 2026 17:40:24 -0700 (PDT) Message-ID: <295f55ea-f5bd-49e0-8eb6-3a6aab9e981f@gmail.com> Date: Wed, 29 Jul 2026 20:40:20 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Demi Marie Obenour Subject: Re: [PATCH v4 02/20] tools: Add control group manager To: Alyssa Ross References: <20260721-cgroups-v4-0-46b2e5fff7b6@gmail.com> <20260721-cgroups-v4-2-46b2e5fff7b6@gmail.com> <87a4rjrp2t.fsf@alyssa.is> <87cxw8ljkh.fsf@alyssa.is> Content-Language: en-US In-Reply-To: <87cxw8ljkh.fsf@alyssa.is> Autocrypt: addr=demiobenour@gmail.com; keydata= xsFNBFp+A0oBEADffj6anl9/BHhUSxGTICeVl2tob7hPDdhHNgPR4C8xlYt5q49yB+l2nipd aq+4Gk6FZfqC825TKl7eRpUjMriwle4r3R0ydSIGcy4M6eb0IcxmuPYfbWpr/si88QKgyGSV Z7GeNW1UnzTdhYHuFlk8dBSmB1fzhEYEk0RcJqg4AKoq6/3/UorR+FaSuVwT7rqzGrTlscnT DlPWgRzrQ3jssesI7sZLm82E3pJSgaUoCdCOlL7MMPCJwI8JpPlBedRpe9tfVyfu3euTPLPx wcV3L/cfWPGSL4PofBtB8NUU6QwYiQ9Hzx4xOyn67zW73/G0Q2vPPRst8LBDqlxLjbtx/WLR 6h3nBc3eyuZ+q62HS1pJ5EvUT1vjyJ1ySrqtUXWQ4XlZyoEFUfpJxJoN0A9HCxmHGVckzTRl 5FMWo8TCniHynNXsBtDQbabt7aNEOaAJdE7to0AH3T/Bvwzcp0ZJtBk0EM6YeMLtotUut7h2 Bkg1b//r6bTBswMBXVJ5H44Qf0+eKeUg7whSC9qpYOzzrm7+0r9F5u3qF8ZTx55TJc2g656C 9a1P1MYVysLvkLvS4H+crmxA/i08Tc1h+x9RRvqba4lSzZ6/Tmt60DPM5Sc4R0nSm9BBff0N m0bSNRS8InXdO1Aq3362QKX2NOwcL5YaStwODNyZUqF7izjK4QARAQABzTxEZW1pIE1hcmll IE9iZW5vdXIgKGxvdmVyIG9mIGNvZGluZykgPGRlbWlvYmVub3VyQGdtYWlsLmNvbT7CwXgE EwECACIFAlp+A0oCGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJELKItV//nCLBhr8Q AK/xrb4wyi71xII2hkFBpT59ObLN+32FQT7R3lbZRjVFjc6yMUjOb1H/hJVxx+yo5gsSj5LS 9AwggioUSrcUKldfA/PKKai2mzTlUDxTcF3vKx6iMXKA6AqwAw4B57ZEJoMM6egm57TV19kz PMc879NV2nc6+elaKl+/kbVeD3qvBuEwsTe2Do3HAAdrfUG/j9erwIk6gha/Hp9yZlCnPTX+ VK+xifQqt8RtMqS5R/S8z0msJMI/ajNU03kFjOpqrYziv6OZLJ5cuKb3bZU5aoaRQRDzkFIR 6aqtFLTohTo20QywXwRa39uFaOT/0YMpNyel0kdOszFOykTEGI2u+kja35g9TkH90kkBTG+a EWttIht0Hy6YFmwjcAxisSakBuHnHuMSOiyRQLu43ej2+mDWgItLZ48Mu0C3IG1seeQDjEYP tqvyZ6bGkf2Vj+L6wLoLLIhRZxQOedqArIk/Sb2SzQYuxN44IDRt+3ZcDqsPppoKcxSyd1Ny 2tpvjYJXlfKmOYLhTWs8nwlAlSHX/c/jz/ywwf7eSvGknToo1Y0VpRtoxMaKW1nvH0OeCSVJ itfRP7YbiRVc2aNqWPCSgtqHAuVraBRbAFLKh9d2rKFB3BmynTUpc1BQLJP8+D5oNyb8Ts4x Xd3iV/uD8JLGJfYZIR7oGWFLP4uZ3tkneDfYzsFNBFp+A0oBEAC9ynZI9LU+uJkMeEJeJyQ/ 8VFkCJQPQZEsIGzOTlPnwvVna0AS86n2Z+rK7R/usYs5iJCZ55/JISWd8xD57ue0eB47bcJv VqGlObI2DEG8TwaW0O0duRhDgzMEL4t1KdRAepIESBEA/iPpI4gfUbVEIEQuqdqQyO4GAe+M kD0Hy5JH/0qgFmbaSegNTdQg5iqYjRZ3ttiswalql1/iSyv1WYeC1OAs+2BLOAT2NEggSiVO txEfgewsQtCWi8H1SoirakIfo45Hz0tk/Ad9ZWh2PvOGt97Ka85o4TLJxgJJqGEnqcFUZnJJ riwoaRIS8N2C8/nEM53jb1sH0gYddMU3QxY7dYNLIUrRKQeNkF30dK7V6JRH7pleRlf+wQcN fRAIUrNlatj9TxwivQrKnC9aIFFHEy/0mAgtrQShcMRmMgVlRoOA5B8RTulRLCmkafvwuhs6 dCxN0GNAORIVVFxjx9Vn7OqYPgwiofZ6SbEl0hgPyWBQvE85klFLZLoj7p+joDY1XNQztmfA rnJ9x+YV4igjWImINAZSlmEcYtd+xy3Li/8oeYDAqrsnrOjb+WvGhCykJk4urBog2LNtcyCj kTs7F+WeXGUo0NDhbd3Z6AyFfqeF7uJ3D5hlpX2nI9no/ugPrrTVoVZAgrrnNz0iZG2DVx46 x913pVKHl5mlYQARAQABwsFfBBgBAgAJBQJafgNKAhsMAAoJELKItV//nCLBwNIP/AiIHE8b oIqReFQyaMzxq6lE4YZCZNj65B/nkDOvodSiwfwjjVVE2V3iEzxMHbgyTCGA67+Bo/d5aQGj gn0TPtsGzelyQHipaUzEyrsceUGWYoKXYyVWKEfyh0cDfnd9diAm3VeNqchtcMpoehETH8fr RHnJdBcjf112PzQSdKC6kqU0Q196c4Vp5HDOQfNiDnTf7gZSj0BraHOByy9LEDCLhQiCmr+2 E0rW4tBtDAn2HkT9uf32ZGqJCn1O+2uVfFhGu6vPE5qkqrbSE8TG+03H8ecU2q50zgHWPdHM OBvy3EhzfAh2VmOSTcRK+tSUe/u3wdLRDPwv/DTzGI36Kgky9MsDC5gpIwNbOJP2G/q1wT1o Gkw4IXfWv2ufWiXqJ+k7HEi2N1sree7Dy9KBCqb+ca1vFhYPDJfhP75I/VnzHVssZ/rYZ9+5 1yDoUABoNdJNSGUYl+Yh9Pw9pE3Kt4EFzUlFZWbE4xKL/NPno+z4J9aWemLLszcYz/u3XnbO vUSQHSrmfOzX3cV4yfmjM5lewgSstoxGyTx2M8enslgdXhPthZlDnTnOT+C+OTsh8+m5tos8 HQjaPM01MKBiAqdPgksm1wu2DrrwUi6ChRVTUBcj6+/9IJ81H2P2gJk3Ls3AVIxIffLoY34E +MYSfkEjBz0E8CLOcAw7JIwAaeBT Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------0l90qDjikH9YgYCFqRKw0XFc" Message-ID-Hash: WUB7ZDI7GBOU5ZK3BYMTUVGSYS53WJBD X-Message-ID-Hash: WUB7ZDI7GBOU5ZK3BYMTUVGSYS53WJBD X-MailFrom: demiobenour@gmail.com X-Mailman-Rule-Hits: member-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; emergency CC: Spectrum OS Development X-Mailman-Version: 3.3.10 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --------------0l90qDjikH9YgYCFqRKw0XFc Content-Type: multipart/mixed; boundary="------------zqFlPFg6Hg31MPizq6vqpZB9"; protected-headers="v1"; hp="clear" Message-ID: <295f55ea-f5bd-49e0-8eb6-3a6aab9e981f@gmail.com> Date: Wed, 29 Jul 2026 20:40:20 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Demi Marie Obenour Subject: Re: [PATCH v4 02/20] tools: Add control group manager To: Alyssa Ross Cc: Spectrum OS Development References: <20260721-cgroups-v4-0-46b2e5fff7b6@gmail.com> <20260721-cgroups-v4-2-46b2e5fff7b6@gmail.com> <87a4rjrp2t.fsf@alyssa.is> <87cxw8ljkh.fsf@alyssa.is> Content-Language: en-US In-Reply-To: <87cxw8ljkh.fsf@alyssa.is> Autocrypt: addr=demiobenour@gmail.com; keydata= xsFNBFp+A0oBEADffj6anl9/BHhUSxGTICeVl2tob7hPDdhHNgPR4C8xlYt5q49yB+l2nipd aq+4Gk6FZfqC825TKl7eRpUjMriwle4r3R0ydSIGcy4M6eb0IcxmuPYfbWpr/si88QKgyGSV Z7GeNW1UnzTdhYHuFlk8dBSmB1fzhEYEk0RcJqg4AKoq6/3/UorR+FaSuVwT7rqzGrTlscnT DlPWgRzrQ3jssesI7sZLm82E3pJSgaUoCdCOlL7MMPCJwI8JpPlBedRpe9tfVyfu3euTPLPx wcV3L/cfWPGSL4PofBtB8NUU6QwYiQ9Hzx4xOyn67zW73/G0Q2vPPRst8LBDqlxLjbtx/WLR 6h3nBc3eyuZ+q62HS1pJ5EvUT1vjyJ1ySrqtUXWQ4XlZyoEFUfpJxJoN0A9HCxmHGVckzTRl 5FMWo8TCniHynNXsBtDQbabt7aNEOaAJdE7to0AH3T/Bvwzcp0ZJtBk0EM6YeMLtotUut7h2 Bkg1b//r6bTBswMBXVJ5H44Qf0+eKeUg7whSC9qpYOzzrm7+0r9F5u3qF8ZTx55TJc2g656C 9a1P1MYVysLvkLvS4H+crmxA/i08Tc1h+x9RRvqba4lSzZ6/Tmt60DPM5Sc4R0nSm9BBff0N m0bSNRS8InXdO1Aq3362QKX2NOwcL5YaStwODNyZUqF7izjK4QARAQABzTxEZW1pIE1hcmll IE9iZW5vdXIgKGxvdmVyIG9mIGNvZGluZykgPGRlbWlvYmVub3VyQGdtYWlsLmNvbT7CwXgE EwECACIFAlp+A0oCGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJELKItV//nCLBhr8Q AK/xrb4wyi71xII2hkFBpT59ObLN+32FQT7R3lbZRjVFjc6yMUjOb1H/hJVxx+yo5gsSj5LS 9AwggioUSrcUKldfA/PKKai2mzTlUDxTcF3vKx6iMXKA6AqwAw4B57ZEJoMM6egm57TV19kz PMc879NV2nc6+elaKl+/kbVeD3qvBuEwsTe2Do3HAAdrfUG/j9erwIk6gha/Hp9yZlCnPTX+ VK+xifQqt8RtMqS5R/S8z0msJMI/ajNU03kFjOpqrYziv6OZLJ5cuKb3bZU5aoaRQRDzkFIR 6aqtFLTohTo20QywXwRa39uFaOT/0YMpNyel0kdOszFOykTEGI2u+kja35g9TkH90kkBTG+a EWttIht0Hy6YFmwjcAxisSakBuHnHuMSOiyRQLu43ej2+mDWgItLZ48Mu0C3IG1seeQDjEYP tqvyZ6bGkf2Vj+L6wLoLLIhRZxQOedqArIk/Sb2SzQYuxN44IDRt+3ZcDqsPppoKcxSyd1Ny 2tpvjYJXlfKmOYLhTWs8nwlAlSHX/c/jz/ywwf7eSvGknToo1Y0VpRtoxMaKW1nvH0OeCSVJ itfRP7YbiRVc2aNqWPCSgtqHAuVraBRbAFLKh9d2rKFB3BmynTUpc1BQLJP8+D5oNyb8Ts4x Xd3iV/uD8JLGJfYZIR7oGWFLP4uZ3tkneDfYzsFNBFp+A0oBEAC9ynZI9LU+uJkMeEJeJyQ/ 8VFkCJQPQZEsIGzOTlPnwvVna0AS86n2Z+rK7R/usYs5iJCZ55/JISWd8xD57ue0eB47bcJv VqGlObI2DEG8TwaW0O0duRhDgzMEL4t1KdRAepIESBEA/iPpI4gfUbVEIEQuqdqQyO4GAe+M kD0Hy5JH/0qgFmbaSegNTdQg5iqYjRZ3ttiswalql1/iSyv1WYeC1OAs+2BLOAT2NEggSiVO txEfgewsQtCWi8H1SoirakIfo45Hz0tk/Ad9ZWh2PvOGt97Ka85o4TLJxgJJqGEnqcFUZnJJ riwoaRIS8N2C8/nEM53jb1sH0gYddMU3QxY7dYNLIUrRKQeNkF30dK7V6JRH7pleRlf+wQcN fRAIUrNlatj9TxwivQrKnC9aIFFHEy/0mAgtrQShcMRmMgVlRoOA5B8RTulRLCmkafvwuhs6 dCxN0GNAORIVVFxjx9Vn7OqYPgwiofZ6SbEl0hgPyWBQvE85klFLZLoj7p+joDY1XNQztmfA rnJ9x+YV4igjWImINAZSlmEcYtd+xy3Li/8oeYDAqrsnrOjb+WvGhCykJk4urBog2LNtcyCj kTs7F+WeXGUo0NDhbd3Z6AyFfqeF7uJ3D5hlpX2nI9no/ugPrrTVoVZAgrrnNz0iZG2DVx46 x913pVKHl5mlYQARAQABwsFfBBgBAgAJBQJafgNKAhsMAAoJELKItV//nCLBwNIP/AiIHE8b oIqReFQyaMzxq6lE4YZCZNj65B/nkDOvodSiwfwjjVVE2V3iEzxMHbgyTCGA67+Bo/d5aQGj gn0TPtsGzelyQHipaUzEyrsceUGWYoKXYyVWKEfyh0cDfnd9diAm3VeNqchtcMpoehETH8fr RHnJdBcjf112PzQSdKC6kqU0Q196c4Vp5HDOQfNiDnTf7gZSj0BraHOByy9LEDCLhQiCmr+2 E0rW4tBtDAn2HkT9uf32ZGqJCn1O+2uVfFhGu6vPE5qkqrbSE8TG+03H8ecU2q50zgHWPdHM OBvy3EhzfAh2VmOSTcRK+tSUe/u3wdLRDPwv/DTzGI36Kgky9MsDC5gpIwNbOJP2G/q1wT1o Gkw4IXfWv2ufWiXqJ+k7HEi2N1sree7Dy9KBCqb+ca1vFhYPDJfhP75I/VnzHVssZ/rYZ9+5 1yDoUABoNdJNSGUYl+Yh9Pw9pE3Kt4EFzUlFZWbE4xKL/NPno+z4J9aWemLLszcYz/u3XnbO vUSQHSrmfOzX3cV4yfmjM5lewgSstoxGyTx2M8enslgdXhPthZlDnTnOT+C+OTsh8+m5tos8 HQjaPM01MKBiAqdPgksm1wu2DrrwUi6ChRVTUBcj6+/9IJ81H2P2gJk3Ls3AVIxIffLoY34E +MYSfkEjBz0E8CLOcAw7JIwAaeBT --------------zqFlPFg6Hg31MPizq6vqpZB9 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 7/27/26 08:10, Alyssa Ross wrote: > Demi Marie Obenour writes: >=20 >> On 7/22/26 12:01, Alyssa Ross wrote: >>> Demi Marie Obenour writes: >=20 >>>> +#[derive(Debug)] >>>> +pub(crate) struct Cgroup { >>>> + path: PathBuf, >>>> + fd: Vec<(OwnedFd, bool)>, >>> >>> There's no point storing all these exclusivity bools, is there? I th= ink >>> only the last one is ever checked, so we could make things tighter an= d >>> clearer like this, where we only track the exclusivity of the last fd= : >> >> Cgroup::enable_subtree_control() checks the exclusivity >> of the caller-provided depth. Line 232 of main.rs calls >> enable_subtree_control(2). >> >> These bools are only used in assertions, so they could be removed. >> I will leave that up to you. The advantage of keeping them is that a >> panic is vastly easier to debug than a race condition due to improper >> locking. >=20 > Can you explain to me why we need to support enabling subtree control > for different depths? Intuitively, I'd expect this program to only eve= r > operate on the cgroup associated with the service that's invoking it. Init runs "cgroup-setup --init-subtree .", which moves PID 1 to /$inner.service and enables all controllers in /. I'll just use sed to enable the controllers, which avoids special cases around paths with no file name. >> I'm very used to writing this kind of code in C, so I went with a C-li= ke >> style instead of using Rust stdlib APIs. I don't like having extra >> abstractions in this kind of code, as it obscures what is going on >> under the hood. That is less important here, but it's very important >> in programs like mount-flatpak. >=20 > I did say myself that my suggestion here might be too clever, because i= t > uses the standard library in a way that's unintuitive. In general, > though, we can expect readers of Rust code to be more familiar with the= > standard library than with byte-by-byte stringy patch processing. Woul= d > still be nicer to take &Path here, I think, even if you then walk > through it byte by byte, just to make it slightly clearer what this doe= s. Will fix in v5. >>>> + pub fn read_control_file(&self, fd: BorrowedFd, p: &Path) -> Re= sult, String> { >>>> + let mut buf =3D Vec::new(); >>>> + let err =3D |e: &dyn Display, p: &Path, msg: &str| { >>>> + let path =3D self.path.join(p); >>>> + format!("Cannot {msg} {path:?}: {e}") >>>> + }; >>>> + File::from(open_subtree_raw(Path::new(p), fd.as_fd()).map_e= rr(|e| err(&e, p, "open"))?) >>> >>> If we're using it for opening files, open_subtree_raw is probably mis= named. >> >> Yup! Do you have a suggestion for improving it? Maybe open_child()? >=20 > open_beneath? Will use in v5. >>>> + pub fn open_sub_cgroup( >>>> + &mut self, >>>> + path: &std::path::Path, >>>> + exclusive: bool, >>>> + allow_missing: bool, >>>> + ) -> Result { >>>> + let mut iter =3D path.components().peekable(); >>>> + while let Some(component) =3D iter.next() { >>> >>> Perhaps would be nicer: >>> >>> let mut components =3D path.components().peekable(); >>> for component in components { >> >> That results in a borrowcheck error. The for loop takes ownership >> of the iterator, but .peek() is called inside the loop. >=20 > Ah, okay. This is fine then. Need is obviated in v5 by a refactor. >>>> + let component =3D match component { >>>> + Component::Normal(component) =3D> component, >>>> + _ =3D> unreachable!(), >>>> + }; >>>> + let sub_fd =3D match self >>>> + .open_sub_cgroup_raw(OFlags::DIRECTORY | OFlags::RD= ONLY, component.as_bytes()) >>>> + { >>>> + Ok(sub_fd) =3D> { >>>> + self.path.push(component); >>> >>> I would really like to not try to store self.path. It seems very >>> complicated to track. It's also very unclear to me from the name (an= d >>> the code) what it is. Is it the path to the cgroup itself, or to its= >>> parent? It looks to me like it should be the cgroup itself, but then= >>> what's going on in purge? >> >> It's the path to the cgroup itself, relative to /sys/fs/cgroup. Its o= nly >> purpose is for logging. >> >>> We could actually improve readability of this quite complicated funct= ion >>> even further if you find it acceptable to just use Errno for the erro= r >>> type. In that case, we'd just return Result<(), Errno>, and callers >>> would check for Errno::NOENT if they wanted to allow missing. Then w= e >>> could just completely drop that argument. In my opinion it would be >>> worth it to move complexity out of here. >> >> I can do this, but it would result in much worse error messages: the >> error would only reference the file name, not the full cgroup path. >> Which would you prefer? >=20 > I would much prefer code I can easily understand. There's always strac= e > for getting the full paths when debugging. This code is dropped in v5. >>>> + pub fn wait_for_empty(&self) -> std::io::Result<()> { >>>> + assert!(self.exclusive()); >>>> + let wait_file =3D self.open_subtree(std::path::Path::new("c= group.events"))?; >>>> + let poll_fd =3D wait_file.as_raw_fd(); >>>> + let mut wait_fd =3D File::from(wait_file); >>>> + let mut fds =3D libc::pollfd { >>>> + fd: poll_fd, >>>> + events: libc::POLLPRI | libc::POLLERR, >>>> + revents: 0, >>>> + }; >>>> + let mut v =3D vec![]; >>>> + loop { >>>> + v.clear(); >>>> + wait_fd >>>> + .seek(std::io::SeekFrom::Start(0)) >>>> + .expect("Seek on control group file should succeed"= ); >>>> + wait_fd >>>> + .read_to_end(&mut v) >>>> + .expect("reading from control group should work"); >>>> + if v.split(|&c| c =3D=3D b'\n').any(|line| line =3D=3D = b"populated 0") { >>>> + break; >>>> + } >>>> + // SAFETY: FFI call, valid arguments, fds contains 1 el= ement >>>> + if unsafe { libc::poll(&raw mut fds, 1, -1) } !=3D 1 { >>>> + panic!("poll failed"); >>>> + } >>>> + } >>> >>> Are you 100% confident that this doesn't race? I don't understand wh= y >>> poll would be triggered in this scenario: >>> >>> 1. "1" is written to cgroup.kill >>> 2. Every process in the cgroup exits and is reaped. >>> 3. cgroup.events is opened, with the cgroup already empty. >>> >>> Are you not relying on 2 happening after 3? Presumably if you open >>> cgroup.events for a cgroup that's already empty, you're not going to = get >>> a poll event to tell you it's empty. >> >> In that case, cgroup.events will include a "populated 0" >> line, so poll will not be called. >=20 > You are correct. :) >=20 > Perhaps could be written more clearly with a while, something like this= : >=20 > while !v.split(|&c| c =3D=3D b'\n').any(|line| line =3D=3D b"popula= ted 0") { > // SAFETY: FFI call, valid arguments, fds contains 1 element > if unsafe { libc::poll(&raw mut fds, 1, -1) } !=3D 1 { > panic!("poll failed"); > } > v.clear(); > wait_fd > .seek(std::io::SeekFrom::Start(0)) > .expect("Seek on control group file should succeed"); > wait_fd > .read_to_end(&mut v) > .expect("reading from control group should work"); > } >=20 > But if you prefer the current way (which avoids a redundant initial > check of an empty Vec, although that should be cheap) that's also fine.= That version would call poll() before checking that the cgroup is empty, and that *can* deadlock :). >>>> + pub(crate) fn make_child(&mut self, path: &Path) -> Result<(), = Errno> { >>>> + assert!(self.exclusive()); >>>> + let component =3D path.as_os_str().as_bytes(); >>>> + assert_single_component(component); >>>> + match rustix::fs::mkdirat( >>>> + self.as_fd(), >>>> + path, >>>> + Mode::RUSR >>>> + | Mode::WUSR >>>> + | Mode::XUSR >>>> + | Mode::RGRP >>>> + | Mode::XGRP >>>> + | Mode::ROTH >>>> + | Mode::XOTH, >>>> + ) { >>>> + Ok(()) | Err(Errno::EXIST) =3D> {} >>>> + bad =3D> return bad, >>>> + } >>>> + let p =3D self.open_sub_cgroup_raw(OFlags::RDONLY | OFlags:= :DIRECTORY, component)?; >>>> + // exclusive lock on parent acts as exclusive lock on child= >>>> + self.fd.push((p, true)); >>>> + self.path.push(path); >>>> + Ok(()) >>>> + } >>>> + >>>> + pub(super) fn purge(&mut self, path: &Path) -> Result<(), Strin= g> { >>> >>> I guess we have to call purge on the parent, rather than on the cgrou= p >>> itself, because of the unlink? Maybe we could call it purge_child? = It >>> confused me for a while. >> >> Correct. Will rename in v5. >> >> The way to understand this code is that Cgroup has two stacks: one >> for file descriptors and one for path components. All operations >> operate at a specified depth from the top of the stack. 1 refers to >> the top of the stack, 2 to one level below that, and so on. >=20 > 1-indexing is a little unintuitive for Rust, no? >=20 >> >> This function is really confusing because it performs multiple pushes >> and pops on the internal file descriptor stack. The specific algorith= m is: >> >> 1. Start with an exclusive lock. >> >> 2. Try to delete the child directly. >> >> 3. If deletion succeeds, or if it fails with ENOENT, return success. >> >> 4. If deletion fails with anything other than EBUSY, return an error.= >> >> 5. Open the child cgroup and take an exclusive lock on it. This push= es >> the child cgroup's FD onto the stack. The open_subtree() method >> also pushes the child path onto the stack. >> >> 6. Take a *shared* lock on the FD that is directly below the top >> of the stack. This is the file descriptor that was initially >> on the top of the stack. >> >> This releases the exclusive lock, allowing other operations on >> different children to proceed. Different operations on the cgroup= >> being purged will be blocked by the exclusive lock taken in step 5= =2E >> >> 7. Kill all programs in the cgroup by writing 1 to cgroup.kill. >> >> 8. Open cgroup.events. >> >> 9. Read from the FD opened in step 8. If the file contains the line >> "populated 0", go to step 11. >> >> 10. Call poll() on the FD opened in step 8 to wait for POLLERR or >> POLLPRI to happen. Then go back to step 9. >> >> This is race-free because the kernel will set the "this is ready" >> flag after every change that affects what would be read from >> the file. >> >> 11. Pop the file descriptor to the being-purged cgroup from the stack.= >> >> 12. Use the just-popped file descriptor to remove all subdirectories >> recursively. Files must not be deleted, as the kernel doesn't >> allow it. Then close the file descriptor, releasing the exclusive= >> lock held on it. >> =20 >> 13. Take an exclusive lock on the *parent* of the cgroup that was just= purged. >> >> This must be done after the file descriptor to the cgroup being >> purged has been closed. Otherwise, there is the potential for >> an ABBA deadlock: another program might hold a shared lock on >> the parent, and be waiting to get an exclusive lock on the child. >> >> 14. Delete the being-purged cgroup. Treat EBUSY and ENOENT as success= : >> the first means that a concurrently-running program re-created the= >> cgroup, while the second means that a concurrently-running program= >> deleted it. The name of the cgroup being purged is currently at >> the top of the path stack. >> >> 15. Pop the name of the cgroup being purged off of the stack. >> =20 >> At the end, self is in the same state it was before the operation. >> >> If you are complaining that this is about as readable as Forth, >> then I agree with you :). >=20 > Could this be made clearer by not mutating self, and either just storin= g > the child stuff in local variables or another Cgroup object? I think a= > big part of the confusion here is that this function temporarily change= s > which cgroup the Cgroup object it's called on refers to while it's > running. That's extremely difficult to reason about. That makes sense. I'll try to move stuff into helper objects. You're correct that the=20 >>>> + Err(e) =3D> return Err(format!("Cannot purge {:?}: {e}"= , self.joined_path(path))), >>>> + } >>>> + if !self.open_sub_cgroup(path, true, true)? { >>>> + return Ok(()); >>>> + } >>>> + >>>> + rustix::fs::flock( >>>> + self.fd[self.fd.len() - 2].0.as_fd(), >>>> + FlockOperation::LockShared, >>> >>> We already must have at least a shared lock on this at this point, no= ? >>> I don't think we need another one. >> >> We actually have an exclusive lock. If it succeeds, >> Cgroup::open_sub_cgroup() pushes a file descriptor onto self.fd. >> Therefore, the fd being locked here is the one that was initially on t= he >> top of the stack. We assert that an exclusive lock is held on that FD= =2E >> >> Waiting for the control group to become empty is a blocking operation,= >> so this downgrades the lock to a shared one. Otherwise, an in-progres= s >> purge of /a/b would prevent /a/c from being created. >=20 > Ah, didn't realise it would downgrade. Makes sense. >>>> +fn remove_recursively(fd: OwnedFd, remaining_depth: usize) -> Resul= t<(), Errno> { >>>> + if remaining_depth < 1 { >>>> + panic!("control groups too deeply nested"); >>>> + } >>>> + let mut d =3D rustix::fs::Dir::new(fd).expect("cannot start ite= rating"); >>>> + while let Some(element) =3D d.next() { >>>> + let element =3D element.expect("Iterating through a cgroup = directory failed?"); >>>> + if element.file_type() !=3D rustix::fs::FileType::Directory= { >>>> + continue; >>>> + } >>>> + >>>> + let remaining_depth =3D remaining_depth - 1; >>>> + let d: &rustix::fs::Dir =3D &d; >>>> + let dirfd =3D d.fd().unwrap(); >>>> + let path =3D element.file_name(); >>>> + remove_all(remaining_depth, dirfd, path)?; >=20 > I'd inline these variables into the function call the extent possible. > Otherwise I have to follow a lot of shuffling around. Will fix in v5. >>>> + } >>>> + drop(d); >>>> + Ok(()) >>>> +} >>>> + >>>> +fn remove_all( >>>> + remaining_depth: usize, >>>> + dirfd: BorrowedFd<'_>, >>>> + path: &std::ffi::CStr, >>>> +) -> Result<(), Errno> { >>>> + if path =3D=3D c"." || path =3D=3D c".." { >>>> + return Ok(()); >>>> + } >=20 > It's a bit weird that calling remove_all on . or .. does not fail. > Maybe would be clearer to move this check to the call site? Will fix in v5, making the code simpler. >>>> + if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok(= ) { >>>> + return Ok(()); >>>> + } >=20 > We could drop this, right? A few extra syscalls, but less to wrap my > head around. Correct. >>>> + let fd =3D rustix::fs::openat2( >>>> + dirfd, >>>> + path, >>>> + OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY | OFlag= s::DIRECTORY, >>>> + Mode::empty(), >>>> + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | Resolve= Flags::NO_XDEV, >>>> + )?; >>>> + remove_recursively(fd, remaining_depth)?; >>>> + rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR)?; >>>> + Ok(()) >>>> +} >>> >>> Could we save a lot of code by calling std::fs::remove_dir_all with a= >>> /proc/self/fd path? It's already documented to ignore symlinks. >> >> I tried, but that tries to delete files too, and that isn't allowed. >=20 > Ah, alright. That could use an explanatory comment. >=20 > How can we be confident we have enough stack for this? Is there a way > it could be done non-recursively, with state on the heap? In Spectrum, the depth is currently limited to a small constant. >>>> + let status =3D parse_digit_string(&args.next().unwrap(), "exit = status")?; >>>> + let signal =3D args.next().unwrap(); >>>> + let signal =3D if status =3D=3D 256 { >>>> + Some(parse_digit_string(&signal, "signal number")?) >>>> + } else { >>>> + None >>>> + }; >>>> + let service =3D args.next().unwrap(); >>>> + >>>> + let (path, mut cgroup) =3D open_relative_cgroup(service)?; >>>> + let cgroup_target =3D Path::new(path.file_name().unwrap()); >>>> + let exit_125 =3D if let Some(signal) =3D signal { >>>> + match signal as libc::c_int { >>>> + libc::SIGBUS >>>> + | libc::SIGFPE >>>> + | libc::SIGABRT >>>> + | libc::SIGTRAP >>>> + | libc::SIGSEGV >>>> + | libc::SIGILL =3D> { >>>> + // Process *crashed*, indicating a *possible exploi= t attempt*. >>>> + // s6 should *not* restart it. This is distinct fr= om a Rust panic, >>>> + // which is much less likely to indicate memory cor= ruption. >>>> + true >>>> + } >>> >>> This has absolutely nothing to do with cgroups. If you want to have >>> some common finish behaviour, a program called cgroup-setup is not th= e >>> place for it. I don't think there's any need for a separate >>> cgroup-s6-finish mode (as opposed to cgroup-purge). >> >> This program is a multi-call binary, so the various things it can do >> aren't necessarily super tightly related. For instance, all of the >> execline binaries can be built as one program, as can most if not >> all busybox applets. When invoked as cgroup-setup or cgroup-purge, >> it indeed only does cgroup-related tasks. cgroup-s6-finish not >> only handles cgroups, but also other tasks related to being an s6 >> finish script. >> >> That said, using this changes behavior in a way that isn't related >> to cgroups, so if it is to be used at all it should be in a separate >> patch series. I'll remove this from v5. >=20 > Thank you. If we want to have a big multi-call binary that does lots o= f > different things, only some of which are cgroup-specific, cgroup-setup > is not the name for that program. No argument there! >>>> +fn local_cgroup() -> Result { >>>> + let mut local_cgroup: Vec =3D std::fs::read("/proc/thread-s= elf/cgroup") >>>> + .map_err(|e| format!("cannot read /proc/thread-self/cgroup:= {e}"))?; >>>> + let local_cgroup_len =3D local_cgroup.len(); >>>> + if local_cgroup_len < 5 >>>> + || local_cgroup[..4] !=3D *b"0::/" >>>> + || local_cgroup[local_cgroup_len - 1] !=3D b'\n' >>>> + || local_cgroup[4..local_cgroup_len - 1].contains(&b'\n') >>> >>> Last time I suggested a clearer way of doing this, but it has instead= >>> got even less clear. >>> >>> (I'm not sure why we'd care if there's a newline specifically, as >>> opposed to any other control character.) >> >> If cgroups v1 is in use, the file can contain multiple lines, one for >> each cgroup the program is in. I also am not sure if starting with >> "0::/" is an invariant in that case. >> >> Using this program with cgroups v1 mounted is user error and will >> never happen on Spectrum, but if this tool is used outside of Spectrum= , >> it could happen. >=20 > I see. It seems like with cgroups v1, it _could_ start with 0::/, but > probably wouldn't. I think it may not be possible to tell from this > file whether cgroups v1 is in use. >=20 > So I suppose it depends what you want to happen if cgroups v1 is in use= =2E > If it looks enough like cgroups v2, do you continue, or do you > explicitly check for cgroups v1? If the latter (sounds more sensible t= o > me), you need to explicitly check for cgroups v1 somehow I think. Can > cgroups v1 and v2 be in use at the same time? If so, checking might be= > complicated, but if not, you can just check what type of filesystem is > mounted at /sys/fs/cgroup, or see if it has a > /sys/fs/cgroup/cgroup.controllers file. In the case of Spectrum, I think it's okay to just panic if cgroups v2 isn't mounted or isn't working properly. It's a bug in either cgroup-setup or the kernel, almost certainly the former. >>>> + { >>>> + // It's possible to get here if the cgroup path contains a = newline, >>>> + // but that never happens in Spectrum. >>>> + return Err(format!( >>>> + "Invalid contents {local_cgroup:?} of /proc/thread-self= /cgroup - \ >>>> + do you have cgroups v1 mounted instead of cgroups v2?"= >>>> + )); >>>> + } >>>> + >>>> + local_cgroup.copy_within(4..local_cgroup_len - 1, 0); >>>> + local_cgroup.truncate(local_cgroup_len - 5); >>>> + let local_cgroup =3D OsString::from_vec(local_cgroup); >>>> + check_path(&local_cgroup).unwrap(); >>> >>> Why do we need to do this? You're worried the kernel is going to sta= rt >>> including .. components in /proc/thread-self/cgroup? >> >> Originally, I was going to create a wrapper around `Path` that >> guaranteed no `.` or `..` components were present. Its constructor >> would have checked this invariant. However, this turned out to be >> more work due to the amount of wrapper functions required. >=20 > Wise not to proceed with that, I think. But I don't think we need this= > particular instance of the check, given it comes from the kernel. Dropped in v5. --=20 Sincerely, Demi Marie Obenour (she/her/hers) --------------zqFlPFg6Hg31MPizq6vqpZB9-- --------------0l90qDjikH9YgYCFqRKw0XFc Content-Type: application/pgp-signature; name="OpenPGP_signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="OpenPGP_signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEopQtqVJW1aeuo9/sszaHOrMp8lMFAmpqnXUACgkQszaHOrMp 8lNfCw/+I9TqS/y+2j1H3CwPn02qfoRIE7LCLW7NahQTKE9dJMMT6WbgHcXUUjvB urDXOiebP4QKjstTx5/zq97OZnsa9eSiYbUkIfR+SD+7dn4MQlebF2Djd6gNzd3d EklQuKXsQGbDulZP4SG/rkO6+KlGazA17qwI6cBrXQ9t1LUFMDlrdPCinC3Fb16e MwhLupzQVIVFs9d1euxbHLBvmJ7EOju2tMNotPZeG01/xvKh7p8WugawidB1PbEZ m94g8WJAR4kvJgCuM8QptdpRynL3dlIOzJwlOjOPEdkpZpwycpbYyMWexPdyQHsN QjNWdToDjbkbf/DIrS0HLRBw5RwEXIW0sPX3BaI24duI4hHxEnNeL9xw780UL44b w9h1bIbo/qE5eqYdXFFtXMXqdPuw0hgFWnWgwknOHgwB4qWvnviUjvG5080dKmds 3eHb31jnCiVSofPy6QsJsVwDzjrLcVBaRu9P6yzjp115+E0A5Q5MnbmlOMGO1RDQ KkuXnJ56LPN6PV3XgiXAFeD+BnH4P49zMdpEvEb9ddgmoRLULE8+jkdOiFaA+qxK 0y6zPaJna4FBAgZ5pkOG0SlWPm0ZA4Jhph2ezLpbYw845MaKUGe/H4hnec4mv3Ap B7I9aaH/7XgqtC0AsfE4LYZICT7yGOuLcjJ++vxjejWUqqqnEZY= =ErPG -----END PGP SIGNATURE----- --------------0l90qDjikH9YgYCFqRKw0XFc--