lkml.org 
[lkml]   [2013]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.


    \
     
     \ /
      Last update: 2013-06-06 10:41    [W:2.811 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site