From nobody Sun Dec 05 19:53:37 2021 X-Original-To: dev-commits-src-main@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 4519718B2FA4; Sun, 5 Dec 2021 19:53:46 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4J6cgt0Cblz4srD; Sun, 5 Dec 2021 19:53:45 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.16.1/8.16.1) with ESMTPS id 1B5JrbLS033993 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Sun, 5 Dec 2021 21:53:40 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 1B5JrbLS033993 Received: (from kostik@localhost) by tom.home (8.16.1/8.16.1/Submit) id 1B5Jrb6R033992; Sun, 5 Dec 2021 21:53:37 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 5 Dec 2021 21:53:37 +0200 From: Konstantin Belousov To: Mike Karels Cc: Jessica Clarke , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: a4e4132fa3bf - main - swapoff(2): replace special device name argument with a structure Message-ID: References: <202112042221.1B4ML7Ov002151@gitrepo.freebsd.org> <6AA150D7-483E-4F11-B35A-23D6F28ECABB@freebsd.org> <9FA0F081-7F17-479B-96D6-F0754A19029B@karels.net> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <9FA0F081-7F17-479B-96D6-F0754A19029B@karels.net> X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.5 X-Spam-Checker-Version: SpamAssassin 3.4.5 (2021-03-20) on tom.home X-Rspamd-Queue-Id: 4J6cgt0Cblz4srD 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 Sun, Dec 05, 2021 at 01:38:52PM -0600, Mike Karels wrote: > On 5 Dec 2021, at 12:56, Jessica Clarke wrote: > > > 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: > >>>> > >>>> On Sun, Dec 05, 2021 at 03:03:26AM +0000, Jessica Clarke wrote: > >>>>> On 4 Dec 2021, at 22:21, Konstantin Belousov wrote: > >>>>>> > >>>>>> The branch main has been updated by kib: > >>>>>> > >>>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=a4e4132fa3bfadb6047fc0fa5f399f4640460300 > >>>>>> > >>>>>> commit a4e4132fa3bfadb6047fc0fa5f399f4640460300 > >>>>>> Author: Konstantin Belousov > >>>>>> AuthorDate: 2021-11-29 16:26:31 +0000 > >>>>>> Commit: Konstantin Belousov > >>>>>> CommitDate: 2021-12-04 22:20:58 +0000 > >>>>>> > >>>>>> swapoff(2): replace special device name argument with a structure > >>>>>> > >>>>>> 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. > >>>>>> > >>>>>> 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(-) > >>>>>> > >>>>>> 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; > >>>>>> > >>>>>> error = priv_check(td, PRIV_SWAPOFF); > >>>>>> if (error) > >>>>>> return (error); > >>>>>> > >>>>>> + /* > >>>>>> + * 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 = fubyte(uap->name); > >>>>>> + switch (probe_byte) { > >>>>>> + case -1: > >>>>>> + return (EFAULT); > >>>>>> + case 0: > >>>>>> + error = copyin(uap->name, &sa, sizeof(sa)); > >>>>>> + if (error != 0) > >>>>>> + return (error); > >>>>>> + if (sa.flags != 0) > >>>>>> + return (EINVAL); > >>>>>> + break; > >>>>>> + default: > >>>>>> + bzero(&sa, sizeof(sa)); > >>>>>> + sa.name = uap->name; > >>>>>> + break; > >>>>>> + } > >>>>> > >>>>> Doesn’t this change the semantics of swapoff("")? > >>>>> > >>>>> 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’t matter, but > >>>>> this approach is ugly. Can we not just define a new syscall rather than > >>>>> this kind of bodge? > >>>> > >>>> Having two swapoff() syscalls is worse, and having them only differ in > >>>> semantic by single flag is kind of crime. > >>>> > >>>> 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. > >>>> > >>>> 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. > >>> > >>> Why’s it worse? It’s 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’t 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’t 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. > >> > >> 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’ll > > need some other weird error response that’s harder to interpret as > > meaning “that variant of this syscall doesn’t exist any more”. > > > >> New syscall allocation should be done only as a last resort, when existing > >> interfaces cannot be adopted for new functionality. > > > > Which this can’t without breaking the existing well-defined semantics, > > as I’ve 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’s better than this approach. > > 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.