Message ID | 20230509165657.1735798-30-kent.overstreet@linux.dev |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp3037044vqo; Tue, 9 May 2023 10:03:59 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ51Vlu7iBmkWaGpL6bdLDI9ON/CS5jCAPzB0dnTyQWllnoqGAZkLFk4j54IN+4Uw65calN6 X-Received: by 2002:a05:6a21:788c:b0:100:5c04:4903 with SMTP id bf12-20020a056a21788c00b001005c044903mr10376183pzc.55.1683651838938; Tue, 09 May 2023 10:03:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683651838; cv=none; d=google.com; s=arc-20160816; b=mbgleMN8ofy1D2O45XG/piQRqkho4TXchxPs0YE+kpx/SN97JvET65pFOIY0m8QQfu SY1gnfEnA65z46d1sDiRHC3cg2rTIiNBR+B+1KXAyG200OINo58XPghay7uCLG0laJID z4JPzqhR8l0YRtu9FLjP1QLq7RbFsxwU1I+BrW8xR87QLtZdSfJkVbb47Ez5X5yp2mua uhjlTxJ5yQK6vjt8S85BRh3dw+tRtSCNi5szEkvplNWaEVXKHz1L/1DsvJazqsMmJg5p d+4xNQfLLfKKOaEdsqePqQuk2QeFZNUhrm8q+QZDVKTqIg+1AMwnqqIwtcs1Fx5TDyoF ligg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=LbLEE4jIbrZpJwXBO7eCGOTCJ14bvYB1GBwSVBBfWTo=; b=w+I56aid/HK6cQWoqNHjSZO8e3eDvGKjLtdh2c71kw0ck0XGv69msJozqHmx9roeAo ok+GkhApDDpzqPWqX4ctPGk59sJYioGb347NFl82ga74N04ba7o2u4emeL9cDP9vtQBd 5xliWtADSFfBp91+kj80RpTGLRl8dF3cQ7gQTvlekNCNC+T+kg651XIGsWLpGxA6idNU 4kqj4HWoO5N86zYWEYTd/b0MmeWBuUysKvxMLEDwMQqC7JE1KFtdFrJjeFeQeQ1/Keal Jc+rt2ycG4vBGDfw6vqwyisxqwyQD7GXjjLxEgCIepiNb+WMrVp/YwnUhOkQFks1GJCz di7g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=Ema++DBy; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i69-20020a639d48000000b0051948f1993fsi1923557pgd.40.2023.05.09.10.03.37; Tue, 09 May 2023 10:03:58 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=Ema++DBy; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235240AbjEIRA3 (ORCPT <rfc822;baris.duru.linux@gmail.com> + 99 others); Tue, 9 May 2023 13:00:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43708 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234983AbjEIQ7T (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 9 May 2023 12:59:19 -0400 Received: from out-33.mta1.migadu.com (out-33.mta1.migadu.com [IPv6:2001:41d0:203:375::21]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6CCBB65A4 for <linux-kernel@vger.kernel.org>; Tue, 9 May 2023 09:57:44 -0700 (PDT) X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1683651459; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=LbLEE4jIbrZpJwXBO7eCGOTCJ14bvYB1GBwSVBBfWTo=; b=Ema++DByNMG9DiB+n947beAEq7g/Q71m9KoPfjHSpli1UraJyM8iCLpH2dVD7dGmlPZham POXN8Vj6rrwc/2I6l2HTF1fjTiqvONuKx/tIEtEaCgCG3vT3KEZtJaEGlgxgJ8gxdR7Ezw AkgHuGvv7tkdEsQ6O1IeMS4LPLXMVs0= From: Kent Overstreet <kent.overstreet@linux.dev> To: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-bcachefs@vger.kernel.org Cc: Kent Overstreet <kent.overstreet@gmail.com>, Kent Overstreet <kent.overstreet@linux.dev> Subject: [PATCH 29/32] lib/string_helpers: string_get_size() now returns characters wrote Date: Tue, 9 May 2023 12:56:54 -0400 Message-Id: <20230509165657.1735798-30-kent.overstreet@linux.dev> In-Reply-To: <20230509165657.1735798-1-kent.overstreet@linux.dev> References: <20230509165657.1735798-1-kent.overstreet@linux.dev> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1765436910393074224?= X-GMAIL-MSGID: =?utf-8?q?1765436910393074224?= |
Series |
bcachefs - a new COW filesystem
|
|
Commit Message
Kent Overstreet
May 9, 2023, 4:56 p.m. UTC
From: Kent Overstreet <kent.overstreet@gmail.com> printbuf now needs to know the number of characters that would have been written if the buffer was too small, like snprintf(); this changes string_get_size() to return the the return value of snprintf(). Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> --- include/linux/string_helpers.h | 4 ++-- lib/string_helpers.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)
Comments
On Tue, May 09, 2023 at 12:56:54PM -0400, Kent Overstreet wrote: > From: Kent Overstreet <kent.overstreet@gmail.com> > > printbuf now needs to know the number of characters that would have been > written if the buffer was too small, like snprintf(); this changes > string_get_size() to return the the return value of snprintf(). Unfortunately, snprintf doesn't return characters written, it return what it TRIED to write, and can cause a lot of problems[1]. This patch would be fine with me if the snprintf was also replaced by scnprintf, which will return the actual string length copied (or 0) *not* including the trailing %NUL. > [...] > @@ -126,8 +126,8 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units, > else > unit = units_str[units][i]; > > - snprintf(buf, len, "%u%s %s", (u32)size, > - tmp, unit); > + return snprintf(buf, len, "%u%s %s", (u32)size, > + tmp, unit); -Kees [1] https://github.com/KSPP/linux/issues/105
On Wed, Jul 12, 2023 at 12:58:54PM -0700, Kees Cook wrote: > On Tue, May 09, 2023 at 12:56:54PM -0400, Kent Overstreet wrote: > > From: Kent Overstreet <kent.overstreet@gmail.com> > > > > printbuf now needs to know the number of characters that would have been > > written if the buffer was too small, like snprintf(); this changes > > string_get_size() to return the the return value of snprintf(). > > Unfortunately, snprintf doesn't return characters written, it return > what it TRIED to write, and can cause a lot of problems[1]. This patch > would be fine with me if the snprintf was also replaced by scnprintf, > which will return the actual string length copied (or 0) *not* including > the trailing %NUL. ...All of which would be solved if we were converting code away from raw char * buffers to a proper string building type. Which I tried to address when I tried to push printbufs upstream, but that turned into a giant exercise in frustration in dealing with maintainers.
On Wed, Jul 12, 2023 at 12:58:54PM -0700, Kees Cook wrote: > On Tue, May 09, 2023 at 12:56:54PM -0400, Kent Overstreet wrote: > > From: Kent Overstreet <kent.overstreet@gmail.com> > > > > printbuf now needs to know the number of characters that would have been > > written if the buffer was too small, like snprintf(); this changes > > string_get_size() to return the the return value of snprintf(). > > Unfortunately, snprintf doesn't return characters written, it return > what it TRIED to write, and can cause a lot of problems[1]. This patch > would be fine with me if the snprintf was also replaced by scnprintf, > which will return the actual string length copied (or 0) *not* including > the trailing %NUL. Anyways, I can't use scnprintf here, printbufs/seq_buf both need the number of characters that would have been written, but I'll update the comment.
On Wed, Jul 12, 2023 at 04:19:31PM -0400, Kent Overstreet wrote: > On Wed, Jul 12, 2023 at 12:58:54PM -0700, Kees Cook wrote: > > On Tue, May 09, 2023 at 12:56:54PM -0400, Kent Overstreet wrote: > > > From: Kent Overstreet <kent.overstreet@gmail.com> > > > > > > printbuf now needs to know the number of characters that would have been > > > written if the buffer was too small, like snprintf(); this changes > > > string_get_size() to return the the return value of snprintf(). > > > > Unfortunately, snprintf doesn't return characters written, it return > > what it TRIED to write, and can cause a lot of problems[1]. This patch > > would be fine with me if the snprintf was also replaced by scnprintf, > > which will return the actual string length copied (or 0) *not* including > > the trailing %NUL. > > ...All of which would be solved if we were converting code away from raw > char * buffers to a proper string building type. > > Which I tried to address when I tried to push printbufs upstream, but > that turned into a giant exercise in frustration in dealing with > maintainers. Heh, yeah, I've been trying to aim people at using seq_buf instead of a long series of snprintf/strlcat/etc calls. Where can I look at how you wired this up to seq_buf/printbuf? I had trouble finding it when I looked before. I'd really like to find a way to do it without leaving around foot-guns for future callers of string_get_size(). :) I found the printbuf series: https://lore.kernel.org/lkml/20220808024128.3219082-1-willy@infradead.org/ It seems there are some nice improvements in there. It'd be really nice if seq_buf could just grow those changes. Adding a static version of seq_buf_init to be used like you have PRINTBUF_EXTERN would be nice (or even a statically sized initializer). And much of the conversions is just changing types and functions. If we can leave all that alone, things become MUCH easier to review, etc, etc. I'd *love* to see an incremental improvement for seq_buf, especially the heap-allocation part.
On Wed, Jul 12, 2023 at 03:38:44PM -0700, Kees Cook wrote: > Heh, yeah, I've been trying to aim people at using seq_buf instead of > a long series of snprintf/strlcat/etc calls. Where can I look at how > you wired this up to seq_buf/printbuf? I had trouble finding it when I > looked before. I'd really like to find a way to do it without leaving > around foot-guns for future callers of string_get_size(). :) > > I found the printbuf series: > https://lore.kernel.org/lkml/20220808024128.3219082-1-willy@infradead.org/ > It seems there are some nice improvements in there. It'd be really nice > if seq_buf could just grow those changes. Adding a static version of > seq_buf_init to be used like you have PRINTBUF_EXTERN would be nice > (or even a statically sized initializer). And much of the conversions > is just changing types and functions. If we can leave all that alone, > things become MUCH easier to review, etc, etc. I'd *love* to see an > incremental improvement for seq_buf, especially the heap-allocation > part. Well, I raised that with Steve way back when I was starting on the conversions of existing code, and I couldn't get any communication out him regarding making those changes to seq_buf. So, I'd _love_ to resurrect that patch series and get it in after the bcachefs merger, but don't expect me to go back and redo everything :) the amount of code in existing seq_buf users is fairly small compared to bcachef's printbuf usage, and what that patch series does in the rest of the kernel anyways. I'd rather save that energy for ditching the seq_file interface and making that just use a printbuf - clean up that bit of API fragmentation.
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h index fae6beaaa2..44148f8feb 100644 --- a/include/linux/string_helpers.h +++ b/include/linux/string_helpers.h @@ -23,8 +23,8 @@ enum string_size_units { STRING_UNITS_2, /* use binary powers of 2^10 */ }; -void string_get_size(u64 size, u64 blk_size, enum string_size_units units, - char *buf, int len); +int string_get_size(u64 size, u64 blk_size, enum string_size_units units, + char *buf, int len); int parse_int_array_user(const char __user *from, size_t count, int **array); diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 230020a2e0..ca36ceba0e 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -32,8 +32,8 @@ * at least 9 bytes and will always be zero terminated. * */ -void string_get_size(u64 size, u64 blk_size, const enum string_size_units units, - char *buf, int len) +int string_get_size(u64 size, u64 blk_size, const enum string_size_units units, + char *buf, int len) { static const char *const units_10[] = { "B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB" @@ -126,8 +126,8 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units, else unit = units_str[units][i]; - snprintf(buf, len, "%u%s %s", (u32)size, - tmp, unit); + return snprintf(buf, len, "%u%s %s", (u32)size, + tmp, unit); } EXPORT_SYMBOL(string_get_size);