Linux Virtualization - May 2017 - [PATCH 2/2] vhost/scsi: Delete error messages for failed memory allocations in five functions

On Sat, May 20, 2017 at 04:32:17PM +0200, SF Markus Elfring
wrote:
> From: Markus Elfring <elfring at users.sourceforge.net>
> Date: Sat, 20 May 2017 15:50:30 +0200
> 
> Omit seven extra messages for memory allocation failures in these
functions.
> 
> This issue was detected by using the Coccinelle software.
> 
> Link:
http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Please include an actual explanation for this change instead of linking
to slides.  Why are you trying to get rid of memory allocation failure
messages?
> Signed-off-by: Markus Elfring <elfring at users.sourceforge.net>
> ---
>  drivers/vhost/scsi.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 650533916c19..49d07950e2e5 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -417,5 +417,4 @@ vhost_scsi_allocate_evt(struct vhost_scsi *vs,
>  	if (!evt) {
> -		vq_err(vq, "Failed to allocate vhost_scsi_evt\n");
#define vq_err(vq, fmt, ...) do {                                  \
                pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
                if ((vq)->error_ctx)                               \
                                eventfd_signal((vq)->error_ctx, 1);\
        } while (0)

You silently dropped the eventfd_signal() call.  Please explain.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20170522/8e65535e/attachment.sig>

>> Link:
http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> 
> Please include an actual explanation for this change instead of linking
> to slides.
Do you care for a bit of code size reduction by removal of questionable
error messages?

> Why are you trying to get rid of memory allocation failure messages?
Do you find information from a Linux allocation failure report sufficient
for any function implementations here?

>> +++ b/drivers/vhost/scsi.c
>> @@ -417,5 +417,4 @@ vhost_scsi_allocate_evt(struct vhost_scsi *vs,
>>  	if (!evt) {
>> -		vq_err(vq, "Failed to allocate vhost_scsi_evt\n");
> 
> #define vq_err(vq, fmt, ...) do {                                  \
>                 pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
>                 if ((vq)->error_ctx)                               \
>                                 eventfd_signal((vq)->error_ctx, 1);\
>         } while (0)
> 
> You silently dropped the eventfd_signal() call.
Do you prefer to preserve this special error handling then?

Regards,
Markus

On Mon, May 22, 2017 at 12:50:39PM +0200, SF Markus Elfring
wrote:
> > Why are you trying to get rid of memory allocation failure messages?
> 
> Do you find information from a Linux allocation failure report sufficient
> for any function implementations here?
If kmalloc() and friends guarantee to print a warning and backtrace on
every allocation failure, then there's no need for error messages in
callers.

That seems like good justification that can go in the commit
description, but I'm not sure if kmalloc() and friends guarantee to show
a message (not just the first time, but for every failed allocation)?
> >> +++ b/drivers/vhost/scsi.c
> >> @@ -417,5 +417,4 @@ vhost_scsi_allocate_evt(struct vhost_scsi *vs,
> >>  	if (!evt) {
> >> -		vq_err(vq, "Failed to allocate vhost_scsi_evt\n");
> > 
> > #define vq_err(vq, fmt, ...) do {                                  \
> >                 pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
> >                 if ((vq)->error_ctx)                              
\
> >                                 eventfd_signal((vq)->error_ctx,
1);\
> >         } while (0)
> > 
> > You silently dropped the eventfd_signal() call.
> 
> Do you prefer to preserve this special error handling then?
Yes, please leave vq_err() calls.

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20170522/d25ae000/attachment-0001.sig>