From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932508AbbJIPq0 (ORCPT ); Fri, 9 Oct 2015 11:46:26 -0400 Received: from mx2.suse.de ([195.135.220.15]:56614 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932351AbbJIPqY (ORCPT ); Fri, 9 Oct 2015 11:46:24 -0400 Date: Fri, 9 Oct 2015 17:46:19 +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: <20151009154619.GR14464@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> <20151008225437.GL14464@wotan.suse.de> 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 Fri, Oct 09, 2015 at 08:46:42AM -0400, Josh Boyer wrote: > On Thu, Oct 8, 2015 at 6:54 PM, Luis R. Rodriguez wrote: > > 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(). > > That reads much clearer to me. Thanks. With that changed: > > Reviewed-by: Josh Boyer Thanks, amended. Luis