SG_IO v4 support ?

[Dmitry V. Levin]

Hi,

On Wed, Jan 21, 2015 at 05:04:44PM +0100, Bart Van Assche wrote:
> Hello,
> 
> If anyone would like to add SG_IO v4 support in strace, the (very
> lightly tested) patch below could be used as a starting point. Please
> note that I'm not familiar with the strace code base.

Thanks, see my comments below.

> Best regards,
> 
> Bart.
> 
> [PATCH] scsi: Add bsg support
> 
> The Linux kernel supports two different versions of the SG_IO API,
> namely v3 and v4. This patch adds support for version 4 of this API.
> At least the sg3_utils package supports version 4 of this API. Version
> 4 of this API is used if /dev/bsg/H:C:I:L is used as device name.
> ---
>  scsi.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 105 insertions(+), 7 deletions(-)
> 
> diff --git a/scsi.c b/scsi.c
> index daf7252..cbc8d7a 100644
> --- a/scsi.c
> +++ b/scsi.c
> @@ -32,6 +32,7 @@
>  
>  # include <sys/ioctl.h>
>  # include <scsi/sg.h>
> +# include <linux/bsg.h>

We usually wrap new linux kernel includes in ifdefs:

#ifdef HAVE_LINUX_BSG_H
# include <linux/bsg.h>
#endif

>  #include "xlat/sg_io_dxfer_direction.h"
>  
> @@ -59,9 +60,8 @@ print_sg_io_buffer(struct tcb *tcp, unsigned char *addr, const unsigned int len)
>  }
>  
>  static void
> -print_sg_io_req(struct tcb *tcp, struct sg_io_hdr *sg_io)
> +print_sg_io_v3_req(struct tcb *tcp, struct sg_io_hdr *sg_io)
>  {
> -	tprintf("{'%c', ", sg_io->interface_id);
>  	printxval(sg_io_dxfer_direction, sg_io->dxfer_direction,
>  		  "SG_DXFER_???");
>  	tprintf(", cmd[%u]=[", sg_io->cmd_len);
> @@ -82,7 +82,7 @@ print_sg_io_req(struct tcb *tcp, struct sg_io_hdr *sg_io)
>  }
>  
>  static void
> -print_sg_io_res(struct tcb *tcp, struct sg_io_hdr *sg_io)
> +print_sg_io_v3_res(struct tcb *tcp, struct sg_io_hdr *sg_io)
>  {
>  	if (sg_io->dxfer_direction == SG_DXFER_FROM_DEV ||
>  	    sg_io->dxfer_direction == SG_DXFER_TO_FROM_DEV) {
> @@ -102,26 +102,124 @@ print_sg_io_res(struct tcb *tcp, struct sg_io_hdr *sg_io)
>  	tprintf("info=%#x}", sg_io->info);
>  }
>  
> +static void
> +print_sg_io_v4_req(struct tcb *tcp, struct sg_io_v4 *sg_io)
> +{
> +	static const struct xlat sgv4_prot[] = {
> +		{ BSG_PROTOCOL_SCSI, "BSG_PROTOCOL_SCSI" },
> +	};
> +	static const struct xlat sgv4_subpr[] = {
> +		{ BSG_SUB_PROTOCOL_SCSI_CMD, "BSG_SUB_PROTOCOL_SCSI_CMD" },
> +		{ BSG_SUB_PROTOCOL_SCSI_TMF, "BSG_SUB_PROTOCOL_SCSI_TMF" },
> +		{ BSG_SUB_PROTOCOL_SCSI_TRANSPORT,
> +		  "BSG_SUB_PROTOCOL_SCSI_TRANSPORT" },
> +	};

We no longer add xlat structures directly into .c source files.
Instead, there are xlat/*.in files that are automatically converted in
xlat/*.h files that in turn are included by .c files.  You can see
examples in the file you are patching.

> +static void
> +print_sg_io_req(struct tcb *tcp, struct sg_io_hdr *sg_io)
> +{
> +	tprintf("{'%c', ", sg_io->interface_id);
> +	switch (sg_io->interface_id) {
> +	case 'S':
> +		print_sg_io_v3_req(tcp, sg_io);
> +		break;
> +	case 'Q':
> +		print_sg_io_v4_req(tcp, (void *)sg_io);
> +		break;
> +	}
> +}
> +
> +static void
> +print_sg_io_res(struct tcb *tcp, struct sg_io_hdr *sg_io)
> +{
> +	switch (sg_io->interface_id) {
> +	case 'S':
> +		print_sg_io_v3_res(tcp, sg_io);
> +		break;
> +	case 'Q':
> +		print_sg_io_v4_res(tcp, (void *)sg_io);
> +		break;
> +	}
> +}

It's usually better to pass a pointer to a union rather than to pass
a pointer to a struct and then cast it to another struct.

> +
>  int
>  scsi_ioctl(struct tcb *tcp, const unsigned int code, long arg)
>  {
>  	switch (code) {
>  	case SG_IO:
>  		if (entering(tcp)) {
> -			struct sg_io_hdr sg_io;
> +			union {
> +				struct sg_io_hdr sg_io_v3;
> +				struct sg_io_v4  sg_io_v4;
> +			} sg_io;
>  
>  			if (umove(tcp, arg, &sg_io) < 0)
>  				tprintf(", %#lx", arg);

This code is unconditionally trying to fetch sg_io_v4 (which is noticeably
larger than sg_io_v3), so if the structure passed to this syscall is
sg_io_v3, there is a change that this umove call would fail.

I'd probably start with fetching only sg_io_v3, check whether it's
actually sg_io_v3 or sg_io_v4, and fetch remaining bytes if it happened
to be sg_io_v4.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20150123/830c0600/attachment.bin>