From nobody Sun Dec 05 18:56:00 2021 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id EB20F18C165A for ; Sun, 5 Dec 2021 18:56:08 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4J6bPN5zdkz4mZ4 for ; Sun, 5 Dec 2021 18:56:08 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wm1-f42.google.com with SMTP id p27-20020a05600c1d9b00b0033bf8532855so6109047wms.3 for ; Sun, 05 Dec 2021 10:56:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=SP2p5cFXfuAFqWIN/p5aRY1iBmLgByWv4eq4RdjD3oA=; b=XRDe6hpMEZckpOHEIGEiuPMTnrrFb8sj6iX+AF7D5IsMYgTWi0fX82W2JWsQ3SIxrc 8BCaley+ZgnjglBynpuip8BmBoLZ9ELFOBk2IP3H9wssWY0krhssyiIwAsKm2TvTRjd4 eaThq+iEaE3G1gxZGrLZqQw40/EoO+4kVsT0YelDys+InHmC/hoEiTLuJTtLmRBtTaCN Z9bzrZKQrUTvlHSego7sNopBSmNPZSxIxWbIdzkhrbv/2HtNSD/7rS71ZKTWppgMik7L TP2UcvLLM/R7BVTAaamWoyaVNQanBYjbRFfZOSftky8Kh29xoo6/W5mqvwDXA3L37CSD t7dg== X-Gm-Message-State: AOAM530FCMRNaRvG03Nr9zMU7yI6jsxAw0nuaLCvIPpV/uWTJr9/3C8m 00XpgOddl+wrIGt82/vt666YxA== X-Google-Smtp-Source: ABdhPJw06RsYGb8D4M7/8VDHA3qCaYq2zUKJ/6LcxaRuJ4Le6bGYyHcNwMcs5nl4CSyKQZBK8xOmPw== X-Received: by 2002:a7b:c007:: with SMTP id c7mr32915069wmb.82.1638730561638; Sun, 05 Dec 2021 10:56:01 -0800 (PST) Received: from smtpclient.apple (global-5-141.nat-2.net.cam.ac.uk. [131.111.5.141]) by smtp.gmail.com with ESMTPSA id l8sm12678619wmc.40.2021.12.05.10.56.01 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 05 Dec 2021 10:56:01 -0800 (PST) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\)) Subject: Re: git: a4e4132fa3bf - main - swapoff(2): replace special device name argument with a structure From: Jessica Clarke In-Reply-To: Date: Sun, 5 Dec 2021 18:56:00 +0000 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <6AA150D7-483E-4F11-B35A-23D6F28ECABB@freebsd.org> References: <202112042221.1B4ML7Ov002151@gitrepo.freebsd.org> To: Konstantin Belousov X-Mailer: Apple Mail (2.3654.120.0.1.13) X-Rspamd-Queue-Id: 4J6bPN5zdkz4mZ4 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On 5 Dec 2021, at 18:51, Konstantin Belousov = wrote: > On Sun, Dec 05, 2021 at 05:14:54PM +0000, Jessica Clarke wrote: >> On 5 Dec 2021, at 13:22, Konstantin Belousov = wrote: >>>=20 >>> On Sun, Dec 05, 2021 at 03:03:26AM +0000, Jessica Clarke wrote: >>>> On 4 Dec 2021, at 22:21, Konstantin Belousov = wrote: >>>>>=20 >>>>> The branch main has been updated by kib: >>>>>=20 >>>>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3Da4e4132fa3bfadb6047fc0fa5f399f46= 40460300 >>>>>=20 >>>>> commit a4e4132fa3bfadb6047fc0fa5f399f4640460300 >>>>> Author: Konstantin Belousov >>>>> AuthorDate: 2021-11-29 16:26:31 +0000 >>>>> Commit: Konstantin Belousov >>>>> CommitDate: 2021-12-04 22:20:58 +0000 >>>>>=20 >>>>> swapoff(2): replace special device name argument with a structure >>>>>=20 >>>>> For compatibility, add a placeholder pointer to the start of the >>>>> added struct swapoff_new_args, and use it to distinguish old vs. = new >>>>> style of syscall invocation. >>>>>=20 >>>>> Reviewed by: markj >>>>> Discussed with: alc >>>>> Sponsored by: The FreeBSD Foundation >>>>> MFC after: 1 week >>>>> Differential revision: https://reviews.freebsd.org/D33165 >>>>> --- >>>>> sys/vm/swap_pager.c | 27 +++++++++++++++++++++++++-- >>>>> sys/vm/swap_pager.h | 8 ++++++++ >>>>> 2 files changed, 33 insertions(+), 2 deletions(-) >>>>>=20 >>>>> diff --git a/sys/vm/swap_pager.c b/sys/vm/swap_pager.c >>>>> index 165373d1b527..dc1df79f4fcd 100644 >>>>> --- a/sys/vm/swap_pager.c >>>>> +++ b/sys/vm/swap_pager.c >>>>> @@ -2491,15 +2491,38 @@ sys_swapoff(struct thread *td, struct = swapoff_args *uap) >>>>> struct vnode *vp; >>>>> struct nameidata nd; >>>>> struct swdevt *sp; >>>>> - int error; >>>>> + struct swapoff_new_args sa; >>>>> + int error, probe_byte; >>>>>=20 >>>>> error =3D priv_check(td, PRIV_SWAPOFF); >>>>> if (error) >>>>> return (error); >>>>>=20 >>>>> + /* >>>>> + * Detect old vs. new-style swapoff(2) syscall. The first >>>>> + * pointer in the memory pointed to by uap->name is NULL for >>>>> + * the new variant. >>>>> + */ >>>>> + probe_byte =3D fubyte(uap->name); >>>>> + switch (probe_byte) { >>>>> + case -1: >>>>> + return (EFAULT); >>>>> + case 0: >>>>> + error =3D copyin(uap->name, &sa, sizeof(sa)); >>>>> + if (error !=3D 0) >>>>> + return (error); >>>>> + if (sa.flags !=3D 0) >>>>> + return (EINVAL); >>>>> + break; >>>>> + default: >>>>> + bzero(&sa, sizeof(sa)); >>>>> + sa.name =3D uap->name; >>>>> + break; >>>>> + } >>>>=20 >>>> Doesn=E2=80=99t this change the semantics of swapoff("")? >>>>=20 >>>> Previously it would fail deterministically, presumably with ENOENT = or >>>> something, but now it reinterprets whatever follows that string in >>>> memory as the new argument structure. It probably doesn=E2=80=99t = matter, but >>>> this approach is ugly. Can we not just define a new syscall rather = than >>>> this kind of bodge? >>>=20 >>> Having two swapoff() syscalls is worse, and having them only differ = in >>> semantic by single flag is kind of crime. >>>=20 >>> I do not see swapoff("") as problematic, we are changing a minor = semantic of >>> the management syscall. I only wanted to avoid flag day for swapoff = binaries. >>>=20 >>> BTW, I considered requiring proper alignment for uap->name, and then = checking >>> the whole uap->name_old_syscall for NULL, but then decided that this = is >>> overkill. If you think that swapoff("") that important, I can add = that >>> additional verification. >>=20 >> Why=E2=80=99s it worse? It=E2=80=99s just a syscall number, you = deprecate the old one >> and move on, we do that for things relatively regularly. This is = really >> not a good solution; harder to use as a caller since the prototype is >> wrong, impossible to ensure you preserve the semantics for the = existing >> interface in all cases, and ugly to implement. You don=E2=80=99t need = a flag >> day for a new syscall, either, you can continue to only use the new >> method for -f for a release and then switch over to the new syscall >> entirely. Or switch over to the new syscall entirely now and fall = back >> on the old syscall if -f isn=E2=80=99t passed. Defining a new syscall = also lets >> you not need the name_old_syscall member in the struct, and gives you = a >> clean, fully-extensible syscall to which future features can be added >> in a backwards-compatible way, rather than forever keeping around = this >> legacy mess. >=20 > I disagree, it is not just a syscall number, it is whole user/kernel > interface that bloats, which means cognitive efforts from anybody = using > this interfaces, and for which we must maintain ABI compatibility. Which is just as true of this approach; you have the same two interfaces here, just smashed together into a single harder-to-use syscall rather than two separate syscalls. Having a separate syscall at least allows the old one to return ENOSYS in the future, whereas if you ever want to deprecate the old interface with this method then you=E2=80=99= ll need some other weird error response that=E2=80=99s harder to interpret = as meaning =E2=80=9Cthat variant of this syscall doesn=E2=80=99t exist any = more=E2=80=9D. > New syscall allocation should be done only as a last resort, when = existing > interfaces cannot be adopted for new functionality. Which this can=E2=80=99t without breaking the existing well-defined = semantics, as I=E2=80=99ve stated. > Good (or rather, bad) example of the uglyness that is backed by the = attitude > that syscalls are free, is whole *at() mess, or specific stat*() mess = (old, > other bsds, pre-ino64, ino64, at, then stat vs fstat, then Linux statx = which > probably fixes the interface ultimately). It=E2=80=99s better than this approach. Jess