Messages in this thread | | | Date | Fri, 9 Oct 2015 00:54:37 +0200 | From | "Luis R. Rodriguez" <> | Subject | Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper |
| |
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 > <mcgrof@do-not-panic.com> wrote: > > From: David Howells <dhowells@redhat.com> > > > > 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 <dhowells@redhat.com> > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> > > 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
| |