From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756066AbbJHWyo (ORCPT ); Thu, 8 Oct 2015 18:54:44 -0400 Received: from mx2.suse.de ([195.135.220.15]:36641 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751898AbbJHWym (ORCPT ); Thu, 8 Oct 2015 18:54:42 -0400 Date: Fri, 9 Oct 2015 00:54:37 +0200 From: "Luis R. Rodriguez" To: Josh Boyer Cc: "Luis R. Rodriguez" , Greg KH , Ming Lei , Jonathan Corbet , "Linux-Kernel@Vger. Kernel. Org" , linux-doc@vger.kernel.org, David Woodhouse , David Howells , Seth Forshee , Rusty Russell , Michal Marek , Matthew Garrett , kyle@kernel.org, linux-security-module , keyrings@linux-nfs.org, Tom Gundersen Subject: Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper Message-ID: <20151008225437.GL14464@wotan.suse.de> References: <1443721449-22882-1-git-send-email-mcgrof@do-not-panic.com> <1443721449-22882-5-git-send-email-mcgrof@do-not-panic.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 08, 2015 at 01:36:53PM -0400, Josh Boyer wrote: > On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez > wrote: > > From: David Howells > > > > We'll want to reuse this same code later in order to > > read two separate types of file contents. This generalizes > > fw_read_file() for reading a file rebrands it as fw_read_file(). > > Er, maybe that should read "...fw_read_file_contents() for reading a > file and rebrands it as fw_read_file()." ? Thanks, corrected. > > This caller lets us pegs arbitrary data onto the target > > buffer and size if the file is found. > > This sentence is somewhat confusing. The data isn't arbitrary. It is > what the caller wants you to read from path. What is arbitrary, at > least in the context of this function, is the path passed to it. > Maybe rewrite this as: > > "The new function allows us to read file contents from arbitrary paths > and return the data and size of the files read." The path is arbitrary but what I meant by arbitrary data is that the data need no longer be firmware, whereas fw_read_file_contents() *did* require passing firmware_class data structures. What this does is it make the possibility of eventually making a more core system data file reader more obvious, so for instance the goal is to later share a reader with: - firmware_class: fw_read_file() - module: kernel_read() - kexec: copy_file_fd() I will clarify this in the commit log and also clarify the path is arbitrary as well as you note. > > While at it this cleans up the exit paths on fw_read_file(). > > > > Signed-off-by: David Howells > > Signed-off-by: Luis R. Rodriguez > > The code changes themselves look fine. Thank you for the review. Can I peg your Acked-by or Reviewed-by? How about this for a change in the commit log: firmware: generalize reading file contents as a helper We'll want to reuse this same code later in order to read two separate types of file contents. This generalizes fw_read_file_contents() for reading a file and rebrands it as fw_read_file(). This new caller is now generic and that path can be arbitrary, the caller is also agnostic to the firmware_class code now, which begs the possibility of code re-use with other similar callers in the kernel. For instance in the future we may want to share a solution with: - firmware_class: fw_read_file() - module: kernel_read() - kexec: copy_file_fd() While at it this also cleans up the exit paths on fw_read_file(). Luis