Re: [net-next rfc V3 7/9] macvtap: allow TUNSETIFF to create multiqueue device

[Jason Wang <jasowang@redhat.com>]

On 06/06/2013 03:26 PM, Michael S. Tsirkin wrote:
> On Thu, Jun 06, 2013 at 03:12:52PM +0800, Jason Wang wrote:
>> On 06/06/2013 02:59 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jun 06, 2013 at 11:13:29AM +0800, Jason Wang wrote:
>>>> On 06/05/2013 06:43 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 05, 2013 at 02:36:30PM +0800, Jason Wang wrote:
>>>>>> Though the queue were in fact created by open(), we still need to add this check
>>>>>> to be compatible with tuntap which can let mgmt software use a single API to
>>>>>> manage queues. This patch only validates the device name and moves the TUNSETIFF
>>>>>> to a helper.
>>>>>>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> The patch is OK, the description is confusing.
>>>>> What you mean is simply:
>>>>>
>>>>> 	Allow IFF_MULTI_QUEUE in TUNSETIFF for macvtap, to match
>>>>> 	tun behaviour.
>>>>>
>>>>> And if you put it like this, I would say make this
>>>>> the last patch in the series, so userspace
>>>>> can use IFF_MULTI_QUEUE to detect new versus old
>>>>> behaviour.
>> [...]
>>>>>> @@ -887,6 +888,44 @@ static void macvtap_put_vlan(struct macvlan_dev *vlan)
>>>>>>  	dev_put(vlan->dev);
>>>>>>  }
>>>>>>  
>>>>>> +static int macvtap_set_iff(struct file *file, struct ifreq __user *ifr_u)
>>>>>> +{
>>>>>> +	struct macvtap_queue *q = file->private_data;
>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>> +	struct inode *inode = file_inode(file);
>>>>>> +	struct net_device *dev, *dev2;
>>>>>> +	struct ifreq ifr;
>>>>>> +
>>>>>> +	if (copy_from_user(&ifr, ifr_u, sizeof(struct ifreq)))
>>>>>> +		return -EFAULT;
>>>>>> +
>>>>>> +	/* To keep the same behavior of tuntap, validate ifr_name */
>>>>> So I'm not sure - why is it important to validate ifr_name here?
>>>>> We ignore the name for all other flags - why is IFF_MULTI_QUEUE
>>>>> special?
>>>> It raises another question, why not validate ifname like tuntap? We
>>>> should warn userspace about their error, otherwise they may create
>>>> queues on the wrong device. In fact I want validate for both, but keep
>>>> the behaviour w/o IFF_MULTI_QUEUE for backward compatibility.
>>> Basically macvtap ignores ifr_name because it doesn't need it.
>>> Making it ignore it without IFF_MULTI_QUEUE but
>>> not with IFF_MULTI_QUEUE seems ugly.
>>>
>>> Do you think we'll need ifr_name at some point?
>>> Why not validate then, when we actually do?
>>>
>>>
>>>
>> If we want to be more compatible with tuntap to simplify userspace codes.
>>
>> E.g: There's a userspace who want to create both taps and macvtaps using
>> the same codes. For tuntap, we can let kernel name the device, so
>> creating a mq device looks like:
>>
>> open()
>> tunsetiff()
>> if_name = tungetiff()
>> tunsetiff(if_name)
>> ...
>> tunsetiff(if_name)
>>
>> For tuntap, if we specifies a wrong ifr_name, kernel will complains.
>> We'd better do the same for macvtap.
>>
>> [...]
> I don't think we need to worry about returning same error to buggy
> applications. Maybe it would make sense if we did it like this
> the first time around, or maybe it won't, but adding
> inconsistency between macvtap interfaces is even worse.
>

Ok, I will drop this patch in next version.