From nobody Sun Dec 05 20:01:13 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 D113A18B4E97 for ; Sun, 5 Dec 2021 20:01:22 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) (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 4J6crf5J5Jz4tf1 for ; Sun, 5 Dec 2021 20:01:22 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wr1-f52.google.com with SMTP id t9so17994286wrx.7 for ; Sun, 05 Dec 2021 12:01:22 -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=bApKeYeHLWuCyPwKBqXwtwsNjmDShWOr1SmLDBY7dig=; b=zxXIkn8s4dJh+kHeibfL2MaGLxutqKN6DoC0lX9TSHji4UtPdh8qZp/YlQtYstxqRS /nwXj28k1IUpbxN+N86DGeDexzC4V4ZwZTXGA0ZXJPEkxJKbRaUbV2vy+z33HBK+/XdY MeNpQC+ylKgz4c8tfzkI5GJAdI5UjOjF5+FKFmEm/LBk09cjpE0M1cTdKI5REzrjcUa2 SmRHWN9LRAJKyx7/RIrDR6Y773e6/WS52U6L0WbEOP1VZWmvQAaopzkr9WE639Hx5jme ha83g3hspepgOaiCivvPTOgKaAV1g9eEZuAVmvZQ8QS4QSVG9m6HHaZJDZZQlNwnAQxh xsyQ== X-Gm-Message-State: AOAM533etMv0f1chSYp7CLFbc+jytOkavPXRT6Mjfs7qRxHv4CKpdM9Z e8sOnIcHULoBGOm/IxEkHI/8uw== X-Google-Smtp-Source: ABdhPJxqAYIMHqxZghhuUwhWHa7je1qQ2/CtKKQQ0/QHnhFDw9rHzeJrF1t31cJf6FKEBNlufQlWiA== X-Received: by 2002:a5d:6e85:: with SMTP id k5mr37776303wrz.545.1638734474644; Sun, 05 Dec 2021 12:01:14 -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 p2sm11499203wmq.23.2021.12.05.12.01.14 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 05 Dec 2021 12:01:14 -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 20:01:13 +0000 Cc: Mike Karels , src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: <2A2C0F57-6F8D-45E2-A4D9-1BED17093A41@freebsd.org> References: <202112042221.1B4ML7Ov002151@gitrepo.freebsd.org> <6AA150D7-483E-4F11-B35A-23D6F28ECABB@freebsd.org> <9FA0F081-7F17-479B-96D6-F0754A19029B@karels.net> To: Konstantin Belousov X-Mailer: Apple Mail (2.3654.120.0.1.13) X-Rspamd-Queue-Id: 4J6crf5J5Jz4tf1 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 19:53, Konstantin Belousov = wrote: > On Sun, Dec 05, 2021 at 01:38:52PM -0600, Mike Karels wrote: >> On 5 Dec 2021, at 12:56, Jessica Clarke wrote: >>=20 >>> 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. >>>=20 >>> 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=99ll >>> 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. >>>=20 >>>> New syscall allocation should be done only as a last resort, when = existing >>>> interfaces cannot be adopted for new functionality. >>>=20 >>> Which this can=E2=80=99t without breaking the existing well-defined = semantics, >>> as I=E2=80=99ve stated. >>>=20 >>>> 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). >>>=20 >>> It=E2=80=99s better than this approach. >>=20 >> I have less resistance to adding new syscalls, and I agree with Jess = that it >> is the right thing to do in this case. Adding a syscall means the = kernel >> supports either old or new interface, so there is no flag day. And = it is >> easier to clean up; maintaining two syscalls should only be needed = for a >> while on -current, and not in any release. > No, we do not do this. We maintain backward compatibility, old = swapoff(2) > would have to live under COMPAT_FREEBSD12 forever. And that=E2=80=99s fine, it=E2=80=99ll remain its own self-contained bit = of code under COMPAT_FREEBSD12 that wraps the new syscall, not mixed in with the still-current syscall. Jess