From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1CB55C4332D for ; Fri, 15 Jan 2021 17:11:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 013DC2333E for ; Fri, 15 Jan 2021 17:11:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1733206AbhAORLJ (ORCPT ); Fri, 15 Jan 2021 12:11:09 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42746 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1733189AbhAORLJ (ORCPT ); Fri, 15 Jan 2021 12:11:09 -0500 Received: from bedivere.hansenpartnership.com (bedivere.hansenpartnership.com [IPv6:2607:fcd0:100:8a00::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CC908C0613C1; Fri, 15 Jan 2021 09:10:28 -0800 (PST) Received: from localhost (localhost [127.0.0.1]) by bedivere.hansenpartnership.com (Postfix) with ESMTP id 218F41280055; Fri, 15 Jan 2021 09:10:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=hansenpartnership.com; s=20151216; t=1610730627; bh=xRezMHxeSl+GO5HaRdmwnmMXlQPWEuyHU3WuQttC27Q=; h=Message-ID:Subject:From:To:Date:In-Reply-To:References:From; b=M0N6HV1gj4oLueTDraK9xEGp4+oVVGfxCI1eXGlsrd/bshdcnu6dxSg95khxrnEra 5jEojYVxEpYQpte/3pde2rdJb0rW9R4vuTWMExT1GyGz8s2kK3RM1U/Dzw6zSoSRz+ HJQYT0SMSwsoX2WSWDSQEYnmh6uf9JfIIE+d9HXE= Received: from bedivere.hansenpartnership.com ([127.0.0.1]) by localhost (bedivere.hansenpartnership.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id tUA6MTpo-UTL; Fri, 15 Jan 2021 09:10:27 -0800 (PST) Received: from jarvis.int.hansenpartnership.com (unknown [IPv6:2601:600:8280:66d1::c447]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by bedivere.hansenpartnership.com (Postfix) with ESMTPSA id C6A6D128005D; Fri, 15 Jan 2021 09:10:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=hansenpartnership.com; s=20151216; t=1610730627; bh=xRezMHxeSl+GO5HaRdmwnmMXlQPWEuyHU3WuQttC27Q=; h=Message-ID:Subject:From:To:Date:In-Reply-To:References:From; b=M0N6HV1gj4oLueTDraK9xEGp4+oVVGfxCI1eXGlsrd/bshdcnu6dxSg95khxrnEra 5jEojYVxEpYQpte/3pde2rdJb0rW9R4vuTWMExT1GyGz8s2kK3RM1U/Dzw6zSoSRz+ HJQYT0SMSwsoX2WSWDSQEYnmh6uf9JfIIE+d9HXE= Message-ID: Subject: Re: [PATCH v5 1/2] tpm: add sysfs exports for all banks of PCR registers From: James Bottomley To: Jarkko Sakkinen Cc: Greg KH , linux-integrity@vger.kernel.org, Mimi Zohar , linux-api@vger.kernel.org Date: Fri, 15 Jan 2021 09:10:25 -0800 In-Reply-To: References: <20210113232634.23242-1-James.Bottomley@HansenPartnership.com> <20210113232634.23242-2-James.Bottomley@HansenPartnership.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.34.4 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-api@vger.kernel.org On Fri, 2021-01-15 at 08:55 +0200, Jarkko Sakkinen wrote: > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: > > On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote: [...] > > > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files. > > > > Hey these interfaces were added after this patch began life. But > > looking at sysfs_emit_at() I've got to say "aah ... don't you guys > > ever read rusty's guide to interfaces?" an interface which takes in > > an absolute page position but returns a relative offset to the > > position it took in is asking for people to get it wrong. You > > should always be consistent about uses for inputs and > > outputs. Basically the only way you can ever use sysfs_emit_at in > > a show routine is as > > > > offset += sysfs_emit_at(buf, offset, ...); > > > > because you always need to track the absolute offset. > > > > It looks like we already have a couple of bugs in the kernel > > introduced by this confusion ... return sysfs_emit() vs return > > sysfs_emit_at() being the most tricky ... > > How is using sysfs_emit() different from using snprintf() for the > caller, ignoring the added safety measures? I'm new to this API. Using the sprintX variants you maintain a cursor pointer, so they all look like char *cursor = buf; ... cursor += sprintX(cursor, "...", ... ... return cursor - buf; So the input is a relative cursor and the output is the additional offset. I'm not claiming it's the best interface but it is hard to get wrong, just that if we're going to force a new interface we should make it much better. with sysfs_emit_at you use an offset "cursor" but it's hard to know without reading the function how to do it because the return is relative rather than absolute. To have an interface it would be hard to misuse, I think the best way would be to take a pointer to the offset and adjust it after use, so sysfs_emit_at(buf, &offset, ...); That way it returns void so you can't use it in place of return sysfs_emit() And you don't have to worry about whether the return is absolute or relative because it adjusts the pointer for you. The whole point about Rusty and interfaces is that if you are going to invent new interfaces you should make them easy to get right and hard to misuse. A function you can't figure out how to use until you read the source is about 2/10 on the rusty scale: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html James