Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
Julia Lawall writes: > On Sat, 2 Jan 2016, SF Markus Elfring wrote: > >> >> Move the jump label directly before the desired log statement >> >> so that the variable "err" will not be checked once more >> >> after it was determined that a function call failed. >> >> Use the identifier "report_failure" instead of the label "err". >> > >> >Why? >> >> I suggest to reconsider the places with which such a jump label >> is connected. >> >> >> > The code was smart enough >> >> Which action should really be performed after a failure was detected >> and handled a bit already? >> >> * Another condition check >> >> * Just additional error logging >> >> >> > and you're making it uglier that it needs to be. >> >> I assume that a software development taste can evolve, can't it? > > So far, you have gotten several down votes for this kind of change, and no > enthusiasm. > > Admittedly, this is a trivial case, because there are no local variables, > but do you actually know the semantics in C of a jump into a block? And > if you do know, do you think that this semantics is common knowledge? And > do you really think that introducing poorly understandable code is really > worth saving an if test of a single variable on a non-critical path? > > Most of the kernel code is not performance critical at the level of a > single if test. So the goal should be for the code to be easy to > understand and robust to change. The code that is performance critical, > you should probably not touch, ever. The people who wrote it knew what > was important and what was not. Very well said! Only optimise something you can measure. I'm dropping this patch. -- Kalle Valo
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
Julia Lawallwrites: > On Sat, 2 Jan 2016, SF Markus Elfring wrote: > >> >> Move the jump label directly before the desired log statement >> >> so that the variable "err" will not be checked once more >> >> after it was determined that a function call failed. >> >> Use the identifier "report_failure" instead of the label "err". >> > >> >Why? >> >> I suggest to reconsider the places with which such a jump label >> is connected. >> >> >> > The code was smart enough >> >> Which action should really be performed after a failure was detected >> and handled a bit already? >> >> * Another condition check >> >> * Just additional error logging >> >> >> > and you're making it uglier that it needs to be. >> >> I assume that a software development taste can evolve, can't it? > > So far, you have gotten several down votes for this kind of change, and no > enthusiasm. > > Admittedly, this is a trivial case, because there are no local variables, > but do you actually know the semantics in C of a jump into a block? And > if you do know, do you think that this semantics is common knowledge? And > do you really think that introducing poorly understandable code is really > worth saving an if test of a single variable on a non-critical path? > > Most of the kernel code is not performance critical at the level of a > single if test. So the goal should be for the code to be easy to > understand and robust to change. The code that is performance critical, > you should probably not touch, ever. The people who wrote it knew what > was important and what was not. Very well said! Only optimise something you can measure. I'm dropping this patch. -- Kalle Valo
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
On Sat, 2 Jan 2016, SF Markus Elfring wrote: > >> Move the jump label directly before the desired log statement > >> so that the variable "err" will not be checked once more > >> after it was determined that a function call failed. > >> Use the identifier "report_failure" instead of the label "err". > > > >Why? > > I suggest to reconsider the places with which such a jump label > is connected. > > > > The code was smart enough > > Which action should really be performed after a failure was detected > and handled a bit already? > > * Another condition check > > * Just additional error logging > > > > and you're making it uglier that it needs to be. > > I assume that a software development taste can evolve, can't it? So far, you have gotten several down votes for this kind of change, and no enthusiasm. Admittedly, this is a trivial case, because there are no local variables, but do you actually know the semantics in C of a jump into a block? And if you do know, do you think that this semantics is common knowledge? And do you really think that introducing poorly understandable code is really worth saving an if test of a single variable on a non-critical path? Most of the kernel code is not performance critical at the level of a single if test. So the goal should be for the code to be easy to understand and robust to change. The code that is performance critical, you should probably not touch, ever. The people who wrote it knew what was important and what was not. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
>> Move the jump label directly before the desired log statement >> so that the variable "err" will not be checked once more >> after it was determined that a function call failed. >> Use the identifier "report_failure" instead of the label "err". > >Why? I suggest to reconsider the places with which such a jump label is connected. > The code was smart enough Which action should really be performed after a failure was detected and handled a bit already? * Another condition check * Just additional error logging > and you're making it uglier that it needs to be. I assume that a software development taste can evolve, can't it? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
>> Move the jump label directly before the desired log statement >> so that the variable "err" will not be checked once more >> after it was determined that a function call failed. >> Use the identifier "report_failure" instead of the label "err". > >Why? I suggest to reconsider the places with which such a jump label is connected. > The code was smart enough Which action should really be performed after a failure was detected and handled a bit already? * Another condition check * Just additional error logging > and you're making it uglier that it needs to be. I assume that a software development taste can evolve, can't it? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
On Sat, 2 Jan 2016, SF Markus Elfring wrote: > >> Move the jump label directly before the desired log statement > >> so that the variable "err" will not be checked once more > >> after it was determined that a function call failed. > >> Use the identifier "report_failure" instead of the label "err". > > > >Why? > > I suggest to reconsider the places with which such a jump label > is connected. > > > > The code was smart enough > > Which action should really be performed after a failure was detected > and handled a bit already? > > * Another condition check > > * Just additional error logging > > > > and you're making it uglier that it needs to be. > > I assume that a software development taste can evolve, can't it? So far, you have gotten several down votes for this kind of change, and no enthusiasm. Admittedly, this is a trivial case, because there are no local variables, but do you actually know the semantics in C of a jump into a block? And if you do know, do you think that this semantics is common knowledge? And do you really think that introducing poorly understandable code is really worth saving an if test of a single variable on a non-critical path? Most of the kernel code is not performance critical at the level of a single if test. So the goal should be for the code to be easy to understand and robust to change. The code that is performance critical, you should probably not touch, ever. The people who wrote it knew what was important and what was not. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
Hello On 1/2/2016 12:33 AM, SF Markus Elfring wrote: From: Markus Elfring Date: Fri, 1 Jan 2016 22:27:20 +0100 This issue was detected by using the Coccinelle software. Move the jump label directly before the desired log statement so that the variable "err" will not be checked once more after it was determined that a function call failed. Use the identifier "report_failure" instead of the label "err". Why? The code was smart enough and you're making it uglier that it needs to be. Signed-off-by: Markus Elfring MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
On Fri, 1 Jan 2016, SF Markus Elfring wrote: > From: Markus Elfring > Date: Fri, 1 Jan 2016 22:27:20 +0100 > > This issue was detected by using the Coccinelle software. > > Move the jump label directly before the desired log statement > so that the variable "err" will not be checked once more > after it was determined that a function call failed. Putting a label inside an if is ugly. julia > Use the identifier "report_failure" instead of the label "err". > > Signed-off-by: Markus Elfring > --- > drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c > b/drivers/net/wireless/marvell/libertas/if_spi.c > index 82c0796..c9ae27e 100644 > --- a/drivers/net/wireless/marvell/libertas/if_spi.c > +++ b/drivers/net/wireless/marvell/libertas/if_spi.c > @@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct > work_struct *work) > ); > if (err) { > netdev_err(priv->dev, "I/O error\n"); > - goto err; > + goto report_failure; > } > > if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) { > err = if_spi_c2h_cmd(card); > if (err) > - goto err; > + goto report_failure; > } > if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) { > err = if_spi_c2h_data(card); > if (err) > - goto err; > + goto report_failure; > } > > /* > @@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct > work_struct *work) > if (hiStatus & IF_SPI_HIST_CARD_EVENT) > if_spi_e2h(card); > > -err: > - if (err) > + if (err) { > +report_failure: > netdev_err(priv->dev, "%s: got error %d\n", __func__, err); > + } > > lbs_deb_leave(LBS_DEB_SPI); > } > -- > 2.6.3 > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
On Fri, 1 Jan 2016, SF Markus Elfring wrote: > From: Markus Elfring> Date: Fri, 1 Jan 2016 22:27:20 +0100 > > This issue was detected by using the Coccinelle software. > > Move the jump label directly before the desired log statement > so that the variable "err" will not be checked once more > after it was determined that a function call failed. Putting a label inside an if is ugly. julia > Use the identifier "report_failure" instead of the label "err". > > Signed-off-by: Markus Elfring > --- > drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c > b/drivers/net/wireless/marvell/libertas/if_spi.c > index 82c0796..c9ae27e 100644 > --- a/drivers/net/wireless/marvell/libertas/if_spi.c > +++ b/drivers/net/wireless/marvell/libertas/if_spi.c > @@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct > work_struct *work) > ); > if (err) { > netdev_err(priv->dev, "I/O error\n"); > - goto err; > + goto report_failure; > } > > if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) { > err = if_spi_c2h_cmd(card); > if (err) > - goto err; > + goto report_failure; > } > if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) { > err = if_spi_c2h_data(card); > if (err) > - goto err; > + goto report_failure; > } > > /* > @@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct > work_struct *work) > if (hiStatus & IF_SPI_HIST_CARD_EVENT) > if_spi_e2h(card); > > -err: > - if (err) > + if (err) { > +report_failure: > netdev_err(priv->dev, "%s: got error %d\n", __func__, err); > + } > > lbs_deb_leave(LBS_DEB_SPI); > } > -- > 2.6.3 > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
Hello On 1/2/2016 12:33 AM, SF Markus Elfring wrote: From: Markus ElfringDate: Fri, 1 Jan 2016 22:27:20 +0100 This issue was detected by using the Coccinelle software. Move the jump label directly before the desired log statement so that the variable "err" will not be checked once more after it was determined that a function call failed. Use the identifier "report_failure" instead of the label "err". Why? The code was smart enough and you're making it uglier that it needs to be. Signed-off-by: Markus Elfring MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/