Re: [PATCH 5/6] fcoe: implement FIP VLAN responder

[Hannes Reinecke <hare@suse.de>]

On 07/04/2016 12:05 PM, Johannes Thumshirn wrote:
> On Mon, Jul 04, 2016 at 10:29:22AM +0200, Hannes Reinecke wrote:
>> When running in VN2VN mode there is no central instance which
>> would send out any FIP VLAN discovery notifications. So this
>> patch adds a new sysfs attribute 'fip_vlan_responder' which
>> will activate a FIP VLAN discovery responder.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
> 
> [...]
> 
>> +/**
>> + * fcoe_ctlr_vlan_send() - Send a FIP VLAN Notification
>> + * @fip: The FCoE controller
>> + * @sub: sub-opcode for vlan notification or vn2vn vlan notification
>> + * @dest: The destination Ethernet MAC address
>> + * @min_len: minimum size of the Ethernet payload to be sent
>> + */
>> +static void fcoe_ctlr_vlan_send(struct fcoe_ctlr *fip,
>> +			      enum fip_vlan_subcode sub,
>> +			      const u8 *dest)
>> +{
>> +	struct sk_buff *skb;
>> +	struct fip_frame {
>> +		struct ethhdr eth;
>> +		struct fip_header fip;
>> +		struct fip_mac_desc mac;
>> +		struct fip_vlan_desc vlan;
>> +	} __packed * frame;
> 
> Hmmm this is the 2nd time fip_frame is defined in  fcoe_ctlr.c. I'd prefere
> having the type definition somewhere else in this file and then use it in
> fcoe_ctlr_vlan_send() and fcoe_ctlr_vn_send().
> 
This appears to be the convention in the file; every function which is
sending a frame on the wire defines a local variable 'struct fip_frame'
which contains the actual frame definition.

Might be that the style can be improved, but that should be a separate
patch.

>> +	size_t len;
>> +	size_t dlen;
>> +
>> +	len = sizeof(*frame);
>> +	dlen = sizeof(frame->mac) + sizeof(frame->vlan);
>> +	len = max(len, sizeof(struct ethhdr));
>> +
>> +	skb = dev_alloc_skb(len);
>> +	if (!skb)
>> +		return;
> 
> dev_alloc_skb() uses GFP_ATOMIC so it's actually not unlikely to fail
> so please return -ENOMEM here, just so the caller knows what happened.
> 
Again, the calling convention of fcoe_ctlr_send_vlan() insists on using
a void return.
Yes, this should be improved, but again with another patch.

So do I need to send two additional patches for updating the infrastructure?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html