netdev - Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B853468.10509@trash.net>
Date:	Wed, 24 Feb 2010 15:15:04 +0100
From:	Patrick McHardy <kaber@...sh.net>
To:	"Williams, Mitch A" <mitch.a.williams@...el.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>
Subject: Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to
 rtnetlink

Williams, Mitch A wrote:
>> From: Patrick McHardy [mailto:kaber@...sh.net]
> [snip]
>> We usually encapsulate lists of the same attribute type in another
>> top-level attribute. Check out the IFLA_VLAN_*_QOS attributes for
>> an example.
>>
>> The interface should also be symetrical, IOW you should dump the
>> same attributes used in the userspace->kernel direction instead
>> of a combined "info" attribute.
> 
> Sheesh, Patrick, where were you three months ago when I first
> posted this stuff? It would have helped a lot if I heard from
> you back then. We've had at least five internal review cycles
> here and nobody caught this, mostly because nobody understands
> it.
> 
> That being said, I'll take another look at the NLA_NESTED stuff
> and see what I can figure out. Do you know of any place (outside
> of the code) where this is documented?  It's particularly
> difficult to follow this code.
> 
> I see your point about symmetrical interfaces, but I'm not sure
> it's the best thing here. We want the user to be able to set these
> attributes independently, without blowing away any other settings.
> If we put all three settings together into one data structure,
> the code flow will end up being much more complicated.
> 
> I'd prefer to leave the data structures as they are, and switch
> to using nested attributes for the status reporting part, i.e.
> what happens when you type 'ip link show'. Would this work for you?
> 
>> It dev_base_lock really correct here? This is running under the RTNL, so
>> changes to the device list can't happen.
>>
> 
> Good catch - I'll pull out the lock.
> 
> 
>>> +		if (ops->ndo_set_vf_mac)
>>> +			err = ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);
>> Shouldn't this indicate an error if the attributes aren't supported?
> 
> Yes. I'll fix this.
> 
>> The casts aren't necessary. But why does struct ifla_vf_vlan use u32
>> for the vlan in the first place?
> 
> I used u32 for all of the values because that's what everything else
> used. All the stuff in iproute2 seems to like u32 size as well.
> 
> The casts aren't necessary for the compiler, but I put them in for
> readability purposes - to make it obvious. I can remove them if 
> they're objectionable.

I just noticed the patch went in without any of these changes.
Are you going to fix this up?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html