Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1242636 > unrolled thread

Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper

Started byJosh Boyer <jwboyer@fedoraproject.org>
First post2015-10-08 19:40 +0200
Last post2015-10-09 17:50 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper Josh Boyer <jwboyer@fedoraproject.org> - 2015-10-08 19:40 +0200
    Re: [PATCH v2 4/5] firmware: generalize reading file contents as a  helper "Luis R. Rodriguez" <mcgrof@suse.com> - 2015-10-09 01:00 +0200
      Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper Josh Boyer <jwboyer@fedoraproject.org> - 2015-10-09 14:50 +0200
        Re: [PATCH v2 4/5] firmware: generalize reading file contents as a  helper "Luis R. Rodriguez" <mcgrof@suse.com> - 2015-10-09 17:50 +0200

#1242636 — Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper

FromJosh Boyer <jwboyer@fedoraproject.org>
Date2015-10-08 19:40 +0200
SubjectRe: [PATCH v2 4/5] firmware: generalize reading file contents as a helper
Message-ID<qhnkC-1SL-13@gated-at.bofh.it>
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()." ?

> 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."

> 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.

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1242831 — Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper

From"Luis R. Rodriguez" <mcgrof@suse.com>
Date2015-10-09 01:00 +0200
SubjectRe: [PATCH v2 4/5] firmware: generalize reading file contents as a helper
Message-ID<qhskh-w9-1@gated-at.bofh.it>
In reply to#1242636
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243317

FromJosh Boyer <jwboyer@fedoraproject.org>
Date2015-10-09 14:50 +0200
Message-ID<qhFhw-2oj-19@gated-at.bofh.it>
In reply to#1242831
On Thu, Oct 8, 2015 at 6:54 PM, Luis R. Rodriguez <mcgrof@suse.com> 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
>> <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().

That reads much clearer to me.  Thanks.  With that changed:

Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243486 — Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper

From"Luis R. Rodriguez" <mcgrof@suse.com>
Date2015-10-09 17:50 +0200
SubjectRe: [PATCH v2 4/5] firmware: generalize reading file contents as a helper
Message-ID<qhI5I-6qA-9@gated-at.bofh.it>
In reply to#1243317
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 <mcgrof@suse.com> 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
> >> <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().
> 
> That reads much clearer to me.  Thanks.  With that changed:
> 
> Reviewed-by: Josh Boyer <jwboyer@fedoraproject.org>

Thanks, amended.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web