Re: [PATCH 3/9] scsi: use external buffer for command logging

[James Bottomley <jbottomley@parallels.com>]

On Thu, 2015-01-08 at 07:43 +0100, Hannes Reinecke wrote:
> Use an external buffer for __scsi_print_command() and
> move command logging over to use the per-cpu logging
> buffer. With that we can guarantee the command always
> will always be formatted in one line.
> So we can even print out a variable length command
> correctly across several lines.
> Finally rename __scsi_print_command() to
> __scsi_format_comment() to better reflect the
> functionality.
> 
> Tested-by: Robert Elliott <elliott@hp.com>
> Reviewed-by: Robert Elliott <elliott@hp.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/ch.c           |   6 +-
>  drivers/scsi/constants.c    |  74 +-----------------
>  drivers/scsi/scsi_logging.c | 182 ++++++++++++++++++++++++++++++++++++++++----
>  drivers/scsi/sr_ioctl.c     |  13 +++-
>  include/scsi/scsi.h         |   3 +
>  include/scsi/scsi_dbg.h     |   6 +-
>  6 files changed, 192 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index 6bac8a7..79e462f 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -195,8 +195,10 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, int cmd_len,
>   retry:
>  	errno = 0;
>  	if (debug) {
> -		DPRINTK("command: ");
> -		__scsi_print_command(cmd, cmd_len);
> +		char logbuf[SCSI_LOG_BUFSIZE];
> +
> +		__scsi_format_command(logbuf, sizeof(logbuf), cmd, cmd_len);
> +		DPRINTK("command: %s", logbuf);

I really hate using an on-stack buffer here ... we're pretty deep in the
call chain already.

Since it's just required for printing a "command: " prefix, why not just
use scsi_print_command()?


> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index fb929fa..e8deb9c 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -188,6 +188,7 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>         struct scsi_sense_hdr sshdr;
>         int result, err = 0, retries = 0;
>         struct request_sense *sense = cgc->sense;
> +       char logbuf[SCSI_LOG_BUFSIZE];

Same issue here.

>         SDev = cd->device;
>  
> @@ -257,14 +258,20 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>                                 /* sense: Invalid command operation code */
>                                 err = -EDRIVE_CANT_DO_THIS;
>  #ifdef DEBUG
> -                       __scsi_print_command(cgc->cmd, CDROM_PACKET_SIZE);
> +                       __scsi_format_command(logbuf, sizeof(logbuf),
> +                                             cgc->cmd, CDROM_PACKET_SIZE);
> +                       sr_printk(KERN_INFO, cd,
> +                                 "CDROM (ioctl) invalid command: %s\n",
> +                                 logbuf);

Looks like scsi_print_command() will work here too.

>                         scsi_print_sense_hdr(cd->device, cd->cdi.name, &sshdr);
>  #endif
>                         break;
>                 default:
> +                       __scsi_format_command(logbuf, sizeof(logbuf),
> +                                             cgc->cmd, CDROM_PACKET_SIZE);
>                         sr_printk(KERN_ERR, cd,
> -                                 "CDROM (ioctl) error, command: ");
> -                       __scsi_print_command(cgc->cmd, CDROM_PACKET_SIZE);
> +                                 "CDROM (ioctl) error, command: %s\n",
> +                                 logbuf);

And realistically here (with the appropriate sdev_print()'s preceding).

>                         scsi_print_sense_hdr(cd->device, cd->cdi.name, &sshdr);
>                         err = -EIO;
>                 }

James