On 10/2/10 9:45 PM, Hillf Danton wrote:
> The sizeof (struct fc_frame_header) in memset seems missing.
> And skb_trim looks unnecessary?
>
> Signed-off-by: Hillf Danton <[email protected]>
> ---
>
> --- o/linux-2.6.36-rc4/drivers/scsi/libfc/fc_frame.c 2010-09-13
> 07:07:38.000000000 +0800
> +++ m/linux-2.6.36-rc4/drivers/scsi/libfc/fc_frame.c 2010-10-03
> 12:36:10.000000000 +0800
> @@ -81,7 +81,9 @@ struct fc_frame *fc_frame_alloc_fill(str
> fill = 4 - fill;
> fp = _fc_frame_alloc(payload_len + fill);
> if (fp) {
> - memset((char *) fr_hdr(fp) + payload_len, 0, fill);
> + if (likely(fill))
> + memset((char *) fr_hdr(fp) + payload_len +
> + sizeof(struct fc_frame_header), 0, fill);
This is correct as is. memset already handles a zero length.
The caller, which should alwasy be the inlined fc_frame_alloc()
already checked for length not being a multiple of 4, which
is the only way that fill can be zero. If length was a constant,
the code to check it is not generated. Besides, even if fill
is zero, memset() will do the right thing.
This just zeros the last 1 to 3 bytes of the frame if there are
padding bytes (fill). The rest of the frame must be set or
zeroed by the caller of fc_frame_alloc(). Since they're usually
set or copied from somewhere, it is expensive and unnecessary
to zero them as well.
So, I don't think this patch helps anything, although it won't hurt either.
> /* trim is OK, we just allocated it so there are no fragments */
> skb_trim(fp_skb(fp),
> payload_len + sizeof(struct fc_frame_header));
The skb trim is to cut off the fill bytes, which we will add back before
sending. Weird, I know, but it makes the length calculations work out
better in the upper layers.
While we're looking at this, just before this code,
the following fill calculation:
fill = payload_len % 4;
if (fill)
fill = 4 - fill;
Could be simply written as:
fill = -payload_len % 4;
Cheers,
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
