* [PATCH] of: Fix issue where code would fall through to error case. @ 2016-11-17 23:25 Moritz Fischer 2016-11-17 23:40 ` Frank Rowand 0 siblings, 1 reply; 10+ messages in thread From: Moritz Fischer @ 2016-11-17 23:25 UTC (permalink / raw) To: linux-kernel Cc: pantelis.antoniou, robh+dt, frowand.list, moritz, devicetree, Moritz Fischer No longer fall through into the error case that prints out an error if no error (err = 0) occurred. Fixes d9181b20a83(of: Add back an error message, restructured) Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> --- drivers/of/resolver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c index 783bd09..785076d 100644 --- a/drivers/of/resolver.c +++ b/drivers/of/resolver.c @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) err = update_usages_of_a_phandle_reference(overlay, prop, phandle); if (err) - break; + goto err_out; } + of_node_put(tree_symbols); + + return 0; + err_out: pr_err("overlay phandle fixup failed: %d\n", err); out: -- 2.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-17 23:25 [PATCH] of: Fix issue where code would fall through to error case Moritz Fischer @ 2016-11-17 23:40 ` Frank Rowand 2016-11-18 0:02 ` Frank Rowand 0 siblings, 1 reply; 10+ messages in thread From: Frank Rowand @ 2016-11-17 23:40 UTC (permalink / raw) To: Moritz Fischer, linux-kernel, robh+dt Cc: pantelis.antoniou, moritz, devicetree On 11/17/16 15:25, Moritz Fischer wrote: > No longer fall through into the error case that prints out > an error if no error (err = 0) occurred. > > Fixes d9181b20a83(of: Add back an error message, restructured) > Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> > --- > drivers/of/resolver.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c > index 783bd09..785076d 100644 > --- a/drivers/of/resolver.c > +++ b/drivers/of/resolver.c > @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) > > err = update_usages_of_a_phandle_reference(overlay, prop, phandle); > if (err) > - break; > + goto err_out; > } > > + of_node_put(tree_symbols); > + > + return 0; > + > err_out: > pr_err("overlay phandle fixup failed: %d\n", err); > out: Thanks for catching that. Rob, please apply. Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> -Frank ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-17 23:40 ` Frank Rowand @ 2016-11-18 0:02 ` Frank Rowand 2016-11-18 0:10 ` Moritz Fischer 0 siblings, 1 reply; 10+ messages in thread From: Frank Rowand @ 2016-11-18 0:02 UTC (permalink / raw) To: Moritz Fischer, linux-kernel, robh+dt Cc: pantelis.antoniou, moritz, devicetree On 11/17/16 15:40, Frank Rowand wrote: > On 11/17/16 15:25, Moritz Fischer wrote: >> No longer fall through into the error case that prints out >> an error if no error (err = 0) occurred. >> >> Fixes d9181b20a83(of: Add back an error message, restructured) >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> >> --- >> drivers/of/resolver.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >> index 783bd09..785076d 100644 >> --- a/drivers/of/resolver.c >> +++ b/drivers/of/resolver.c >> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >> >> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >> if (err) >> - break; >> + goto err_out; >> } >> >> + of_node_put(tree_symbols); >> + >> + return 0; >> + >> err_out: >> pr_err("overlay phandle fixup failed: %d\n", err); >> out: > > Thanks for catching that. > > Rob, please apply. > > Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> > > -Frank On second thought, isn't the common pattern when clean up is needed for both the no-error path and the error path something like: out: of_node_put(tree_symbols); return err; err_out: pr_err("overlay phandle fixup failed: %d\n", err); goto out; } I don't have a strong opinion, whatever Rob wants to take is fine with me. -Frank ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-18 0:02 ` Frank Rowand @ 2016-11-18 0:10 ` Moritz Fischer 2016-11-23 21:58 ` Rob Herring 0 siblings, 1 reply; 10+ messages in thread From: Moritz Fischer @ 2016-11-18 0:10 UTC (permalink / raw) To: Frank Rowand Cc: Moritz Fischer, linux-kernel, robh+dt, pantelis.antoniou, moritz, devicetree On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: > On 11/17/16 15:40, Frank Rowand wrote: >> On 11/17/16 15:25, Moritz Fischer wrote: >>> No longer fall through into the error case that prints out >>> an error if no error (err = 0) occurred. >>> >>> Fixes d9181b20a83(of: Add back an error message, restructured) >>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> >>> --- >>> drivers/of/resolver.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>> index 783bd09..785076d 100644 >>> --- a/drivers/of/resolver.c >>> +++ b/drivers/of/resolver.c >>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >>> >>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >>> if (err) >>> - break; >>> + goto err_out; >>> } >>> >>> + of_node_put(tree_symbols); >>> + >>> + return 0; >>> + >>> err_out: >>> pr_err("overlay phandle fixup failed: %d\n", err); >>> out: >> >> Thanks for catching that. >> >> Rob, please apply. >> >> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> >> >> -Frank > > On second thought, isn't the common pattern when clean up is needed for > both the no-error path and the error path something like: > > > out: > of_node_put(tree_symbols); > return err; > > err_out: > pr_err("overlay phandle fixup failed: %d\n", err); > goto out; > } > > > I don't have a strong opinion, whatever Rob wants to take is fine with me. Same here. I tried to avoid the jumping back part, but if that's the common pattern, I can submit a v2 doing that instead. Cheers, Moritz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-18 0:10 ` Moritz Fischer @ 2016-11-23 21:58 ` Rob Herring 2016-11-26 21:39 ` Frank Rowand 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2016-11-23 21:58 UTC (permalink / raw) To: Moritz Fischer Cc: Frank Rowand, Moritz Fischer, linux-kernel@vger.kernel.org, Pantelis Antoniou, moritz, devicetree@vger.kernel.org On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer <moritz.fischer.private@gmail.com> wrote: > On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/17/16 15:40, Frank Rowand wrote: >>> On 11/17/16 15:25, Moritz Fischer wrote: >>>> No longer fall through into the error case that prints out >>>> an error if no error (err = 0) occurred. >>>> >>>> Fixes d9181b20a83(of: Add back an error message, restructured) >>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> >>>> --- >>>> drivers/of/resolver.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>>> index 783bd09..785076d 100644 >>>> --- a/drivers/of/resolver.c >>>> +++ b/drivers/of/resolver.c >>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >>>> >>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >>>> if (err) >>>> - break; >>>> + goto err_out; >>>> } >>>> >>>> + of_node_put(tree_symbols); >>>> + >>>> + return 0; >>>> + >>>> err_out: >>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>> out: >>> >>> Thanks for catching that. >>> >>> Rob, please apply. >>> >>> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> >>> >>> -Frank >> >> On second thought, isn't the common pattern when clean up is needed for >> both the no-error path and the error path something like: >> >> >> out: >> of_node_put(tree_symbols); >> return err; >> >> err_out: >> pr_err("overlay phandle fixup failed: %d\n", err); >> goto out; >> } >> >> >> I don't have a strong opinion, whatever Rob wants to take is fine with me. > > Same here. I tried to avoid the jumping back part, but if that's the > common pattern, > I can submit a v2 doing that instead. Both are ugly. Just do: if (err) pr_err(...); Rob ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-23 21:58 ` Rob Herring @ 2016-11-26 21:39 ` Frank Rowand 2016-11-28 15:30 ` Frank Rowand 0 siblings, 1 reply; 10+ messages in thread From: Frank Rowand @ 2016-11-26 21:39 UTC (permalink / raw) To: Rob Herring, Moritz Fischer Cc: Moritz Fischer, linux-kernel@vger.kernel.org, Pantelis Antoniou, moritz, devicetree@vger.kernel.org On 11/23/16 13:58, Rob Herring wrote: > On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer > <moritz.fischer.private@gmail.com> wrote: >> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: >>> On 11/17/16 15:40, Frank Rowand wrote: >>>> On 11/17/16 15:25, Moritz Fischer wrote: >>>>> No longer fall through into the error case that prints out >>>>> an error if no error (err = 0) occurred. >>>>> >>>>> Fixes d9181b20a83(of: Add back an error message, restructured) >>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> >>>>> --- >>>>> drivers/of/resolver.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>>>> index 783bd09..785076d 100644 >>>>> --- a/drivers/of/resolver.c >>>>> +++ b/drivers/of/resolver.c >>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >>>>> >>>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >>>>> if (err) >>>>> - break; >>>>> + goto err_out; >>>>> } >>>>> >>>>> + of_node_put(tree_symbols); >>>>> + >>>>> + return 0; >>>>> + >>>>> err_out: >>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>> out: >>>> >>>> Thanks for catching that. >>>> >>>> Rob, please apply. >>>> >>>> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> >>>> >>>> -Frank >>> >>> On second thought, isn't the common pattern when clean up is needed for >>> both the no-error path and the error path something like: >>> >>> >>> out: >>> of_node_put(tree_symbols); >>> return err; >>> >>> err_out: >>> pr_err("overlay phandle fixup failed: %d\n", err); >>> goto out; >>> } >>> >>> >>> I don't have a strong opinion, whatever Rob wants to take is fine with me. >> >> Same here. I tried to avoid the jumping back part, but if that's the >> common pattern, >> I can submit a v2 doing that instead. > > Both are ugly. Just do: > > if (err) > pr_err(...); > > Rob Agreed. Thanks for the touch of sanity Rob. -Frank ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-26 21:39 ` Frank Rowand @ 2016-11-28 15:30 ` Frank Rowand 2016-11-29 15:06 ` Rob Herring 0 siblings, 1 reply; 10+ messages in thread From: Frank Rowand @ 2016-11-28 15:30 UTC (permalink / raw) To: Rob Herring, Moritz Fischer Cc: Moritz Fischer, linux-kernel@vger.kernel.org, Pantelis Antoniou, moritz, devicetree@vger.kernel.org On 11/26/16 13:39, Frank Rowand wrote: > On 11/23/16 13:58, Rob Herring wrote: >> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer >> <moritz.fischer.private@gmail.com> wrote: >>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: >>>> On 11/17/16 15:40, Frank Rowand wrote: >>>>> On 11/17/16 15:25, Moritz Fischer wrote: >>>>>> No longer fall through into the error case that prints out >>>>>> an error if no error (err = 0) occurred. >>>>>> >>>>>> Fixes d9181b20a83(of: Add back an error message, restructured) >>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> >>>>>> --- >>>>>> drivers/of/resolver.c | 6 +++++- >>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>>>>> index 783bd09..785076d 100644 >>>>>> --- a/drivers/of/resolver.c >>>>>> +++ b/drivers/of/resolver.c >>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >>>>>> >>>>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >>>>>> if (err) >>>>>> - break; >>>>>> + goto err_out; >>>>>> } >>>>>> >>>>>> + of_node_put(tree_symbols); >>>>>> + >>>>>> + return 0; >>>>>> + >>>>>> err_out: >>>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>>> out: >>>>> >>>>> Thanks for catching that. >>>>> >>>>> Rob, please apply. >>>>> >>>>> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> >>>>> >>>>> -Frank >>>> >>>> On second thought, isn't the common pattern when clean up is needed for >>>> both the no-error path and the error path something like: >>>> >>>> >>>> out: >>>> of_node_put(tree_symbols); >>>> return err; >>>> >>>> err_out: >>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>> goto out; >>>> } >>>> >>>> >>>> I don't have a strong opinion, whatever Rob wants to take is fine with me. >>> >>> Same here. I tried to avoid the jumping back part, but if that's the >>> common pattern, >>> I can submit a v2 doing that instead. >> >> Both are ugly. Just do: >> >> if (err) >> pr_err(...); >> >> Rob > > Agreed. Thanks for the touch of sanity Rob. > > -Frank I succumbed to looking only at the few lines of code above and not the fuller context of the file that the patch applies to. The proposed patch was fixing the problem that a normal completion of the for loop was falling through into the err_out label. So what looks cleaner ("if (err) pr_err(...)") is actually not correct. -Frank ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-28 15:30 ` Frank Rowand @ 2016-11-29 15:06 ` Rob Herring 2016-11-29 15:55 ` Moritz Fischer 2016-11-29 16:52 ` Frank Rowand 0 siblings, 2 replies; 10+ messages in thread From: Rob Herring @ 2016-11-29 15:06 UTC (permalink / raw) To: Frank Rowand Cc: Moritz Fischer, Moritz Fischer, linux-kernel@vger.kernel.org, Pantelis Antoniou, moritz, devicetree@vger.kernel.org On Mon, Nov 28, 2016 at 9:30 AM, Frank Rowand <frowand.list@gmail.com> wrote: > On 11/26/16 13:39, Frank Rowand wrote: >> On 11/23/16 13:58, Rob Herring wrote: >>> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer >>> <moritz.fischer.private@gmail.com> wrote: >>>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: >>>>> On 11/17/16 15:40, Frank Rowand wrote: >>>>>> On 11/17/16 15:25, Moritz Fischer wrote: >>>>>>> No longer fall through into the error case that prints out >>>>>>> an error if no error (err = 0) occurred. >>>>>>> >>>>>>> Fixes d9181b20a83(of: Add back an error message, restructured) >>>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> >>>>>>> --- >>>>>>> drivers/of/resolver.c | 6 +++++- >>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>>>>>> index 783bd09..785076d 100644 >>>>>>> --- a/drivers/of/resolver.c >>>>>>> +++ b/drivers/of/resolver.c >>>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >>>>>>> >>>>>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >>>>>>> if (err) >>>>>>> - break; >>>>>>> + goto err_out; >>>>>>> } >>>>>>> >>>>>>> + of_node_put(tree_symbols); >>>>>>> + >>>>>>> + return 0; >>>>>>> + >>>>>>> err_out: >>>>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>>>> out: >>>>>> >>>>>> Thanks for catching that. >>>>>> >>>>>> Rob, please apply. >>>>>> >>>>>> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> >>>>>> >>>>>> -Frank >>>>> >>>>> On second thought, isn't the common pattern when clean up is needed for >>>>> both the no-error path and the error path something like: >>>>> >>>>> >>>>> out: >>>>> of_node_put(tree_symbols); >>>>> return err; >>>>> >>>>> err_out: >>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>> goto out; >>>>> } >>>>> >>>>> >>>>> I don't have a strong opinion, whatever Rob wants to take is fine with me. >>>> >>>> Same here. I tried to avoid the jumping back part, but if that's the >>>> common pattern, >>>> I can submit a v2 doing that instead. >>> >>> Both are ugly. Just do: >>> >>> if (err) >>> pr_err(...); >>> >>> Rob >> >> Agreed. Thanks for the touch of sanity Rob. >> >> -Frank > > I succumbed to looking only at the few lines of code above and not the > fuller context of the file that the patch applies to. > > The proposed patch was fixing the problem that a normal completion > of the for loop was falling through into the err_out label. So what > looks cleaner ("if (err) pr_err(...)") is actually not correct. What!? The *only* problem was printing the error message in the err=0 case. All that needs to be fixed is not doing that. If we do that, then we really only need 1 goto label. Rob ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-29 15:06 ` Rob Herring @ 2016-11-29 15:55 ` Moritz Fischer 2016-11-29 16:52 ` Frank Rowand 1 sibling, 0 replies; 10+ messages in thread From: Moritz Fischer @ 2016-11-29 15:55 UTC (permalink / raw) To: Rob Herring Cc: Frank Rowand, Moritz Fischer, linux-kernel@vger.kernel.org, Pantelis Antoniou, moritz, devicetree@vger.kernel.org Hi Rob, On Tue, Nov 29, 2016 at 09:06:08AM -0600, Rob Herring wrote: > On Mon, Nov 28, 2016 at 9:30 AM, Frank Rowand <frowand.list@gmail.com> wrote: > > On 11/26/16 13:39, Frank Rowand wrote: > >> On 11/23/16 13:58, Rob Herring wrote: > >>> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer > >>> <moritz.fischer.private@gmail.com> wrote: > >>>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: > >>>>> On 11/17/16 15:40, Frank Rowand wrote: > >>>>>> On 11/17/16 15:25, Moritz Fischer wrote: > >>>>>>> No longer fall through into the error case that prints out > >>>>>>> an error if no error (err = 0) occurred. > >>>>>>> > >>>>>>> Fixes d9181b20a83(of: Add back an error message, restructured) > >>>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> > >>>>>>> --- > >>>>>>> drivers/of/resolver.c | 6 +++++- > >>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c > >>>>>>> index 783bd09..785076d 100644 > >>>>>>> --- a/drivers/of/resolver.c > >>>>>>> +++ b/drivers/of/resolver.c > >>>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) > >>>>>>> > >>>>>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); > >>>>>>> if (err) > >>>>>>> - break; > >>>>>>> + goto err_out; > >>>>>>> } > >>>>>>> > >>>>>>> + of_node_put(tree_symbols); > >>>>>>> + > >>>>>>> + return 0; > >>>>>>> + > >>>>>>> err_out: > >>>>>>> pr_err("overlay phandle fixup failed: %d\n", err); > >>>>>>> out: > >>>>>> > >>>>>> Thanks for catching that. > >>>>>> > >>>>>> Rob, please apply. > >>>>>> > >>>>>> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> > >>>>>> > >>>>>> -Frank > >>>>> > >>>>> On second thought, isn't the common pattern when clean up is needed for > >>>>> both the no-error path and the error path something like: > >>>>> > >>>>> > >>>>> out: > >>>>> of_node_put(tree_symbols); > >>>>> return err; > >>>>> > >>>>> err_out: > >>>>> pr_err("overlay phandle fixup failed: %d\n", err); > >>>>> goto out; > >>>>> } > >>>>> > >>>>> > >>>>> I don't have a strong opinion, whatever Rob wants to take is fine with me. > >>>> > >>>> Same here. I tried to avoid the jumping back part, but if that's the > >>>> common pattern, > >>>> I can submit a v2 doing that instead. > >>> > >>> Both are ugly. Just do: > >>> > >>> if (err) > >>> pr_err(...); > >>> > >>> Rob > >> > >> Agreed. Thanks for the touch of sanity Rob. > >> > >> -Frank > > > > I succumbed to looking only at the few lines of code above and not the > > fuller context of the file that the patch applies to. > > > > The proposed patch was fixing the problem that a normal completion > > of the for loop was falling through into the err_out label. So what > > looks cleaner ("if (err) pr_err(...)") is actually not correct. > > What!? The *only* problem was printing the error message in the err=0 > case. All that needs to be fixed is not doing that. If we do that, > then we really only need 1 goto label. I think you're right. Can you look at my v3 that I sent. I also tried to fix cases where we can just do return 0; vs. err = 0; goto err ... err: of_node_put(NULL /*tree_symbols is NULL*/); return err; Thanks, Moritz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Fix issue where code would fall through to error case. 2016-11-29 15:06 ` Rob Herring 2016-11-29 15:55 ` Moritz Fischer @ 2016-11-29 16:52 ` Frank Rowand 1 sibling, 0 replies; 10+ messages in thread From: Frank Rowand @ 2016-11-29 16:52 UTC (permalink / raw) To: Rob Herring Cc: Moritz Fischer, Moritz Fischer, linux-kernel@vger.kernel.org, Pantelis Antoniou, moritz, devicetree@vger.kernel.org On 11/29/16 07:06, Rob Herring wrote: > On Mon, Nov 28, 2016 at 9:30 AM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/26/16 13:39, Frank Rowand wrote: >>> On 11/23/16 13:58, Rob Herring wrote: >>>> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer >>>> <moritz.fischer.private@gmail.com> wrote: >>>>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: >>>>>> On 11/17/16 15:40, Frank Rowand wrote: >>>>>>> On 11/17/16 15:25, Moritz Fischer wrote: >>>>>>>> No longer fall through into the error case that prints out >>>>>>>> an error if no error (err = 0) occurred. >>>>>>>> >>>>>>>> Fixes d9181b20a83(of: Add back an error message, restructured) >>>>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com> >>>>>>>> --- >>>>>>>> drivers/of/resolver.c | 6 +++++- >>>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>>>>>>> index 783bd09..785076d 100644 >>>>>>>> --- a/drivers/of/resolver.c >>>>>>>> +++ b/drivers/of/resolver.c >>>>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >>>>>>>> >>>>>>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >>>>>>>> if (err) >>>>>>>> - break; >>>>>>>> + goto err_out; >>>>>>>> } >>>>>>>> >>>>>>>> + of_node_put(tree_symbols); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> + >>>>>>>> err_out: >>>>>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>>>>> out: >>>>>>> >>>>>>> Thanks for catching that. >>>>>>> >>>>>>> Rob, please apply. >>>>>>> >>>>>>> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com> >>>>>>> >>>>>>> -Frank >>>>>> >>>>>> On second thought, isn't the common pattern when clean up is needed for >>>>>> both the no-error path and the error path something like: >>>>>> >>>>>> >>>>>> out: >>>>>> of_node_put(tree_symbols); >>>>>> return err; >>>>>> >>>>>> err_out: >>>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>>> goto out; >>>>>> } >>>>>> >>>>>> >>>>>> I don't have a strong opinion, whatever Rob wants to take is fine with me. >>>>> >>>>> Same here. I tried to avoid the jumping back part, but if that's the >>>>> common pattern, >>>>> I can submit a v2 doing that instead. >>>> >>>> Both are ugly. Just do: >>>> >>>> if (err) >>>> pr_err(...); >>>> >>>> Rob >>> >>> Agreed. Thanks for the touch of sanity Rob. >>> >>> -Frank >> >> I succumbed to looking only at the few lines of code above and not the >> fuller context of the file that the patch applies to. >> >> The proposed patch was fixing the problem that a normal completion >> of the for loop was falling through into the err_out label. So what >> looks cleaner ("if (err) pr_err(...)") is actually not correct. > > What!? The *only* problem was printing the error message in the err=0 > case. All that needs to be fixed is not doing that. If we do that, > then we really only need 1 goto label. > > Rob I misread your original suggestion to mean to put the "if (err) pr_err(...)" inside the for loop, where Moritz had made his changes. Now I understand what you really meant, to put the "if (err) pr_err(...)" after the for loop. Yes, that is a good way to do it. Sorry for the extra noise.... -Frank ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-11-29 16:53 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-17 23:25 [PATCH] of: Fix issue where code would fall through to error case Moritz Fischer 2016-11-17 23:40 ` Frank Rowand 2016-11-18 0:02 ` Frank Rowand 2016-11-18 0:10 ` Moritz Fischer 2016-11-23 21:58 ` Rob Herring 2016-11-26 21:39 ` Frank Rowand 2016-11-28 15:30 ` Frank Rowand 2016-11-29 15:06 ` Rob Herring 2016-11-29 15:55 ` Moritz Fischer 2016-11-29 16:52 ` Frank Rowand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).