Messages in this thread | | | Date | Thu, 06 Jun 2013 15:31:52 +0800 | From | Jason Wang <> | Subject | Re: [net-next rfc V3 7/9] macvtap: allow TUNSETIFF to create multiqueue device |
| |
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.
| |