[Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap,dev= option

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Dec 09, 2009 at 01:33:36PM +0100, Arnd Bergmann wrote:
> On Wednesday 09 December 2009, Michael S. Tsirkin wrote:
> > On Tue, Dec 08, 2009 at 06:41:44PM +0100, Arnd Bergmann wrote:
> > > --- a/net/tap-bsd.c
> > > +++ b/net/tap-bsd.c
> > > @@ -40,7 +40,8 @@
> > >  #include <util.h>
> > >  #endif
> > >  
> > > -int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
> > > +int tap_open(char *ifname, int ifname_size, char *dev, int dev_size,
> > > +			int *vnet_hdr, int vnet_hdr_required)
> > >  {
> > >      int fd;
> > >      char *dev;
> > 
> > Does this compile?
> 
> I don't have a BSD or Solaris machine here, or even just a cross-compiler, so
> I could not test.

It should be possible to install in a VM if you really want to,
but that's not my point.

> However, I only add two arguments and I did the identical
> change in the header file and the linux version of this file, so I am reasonably
> confident.

'char *dev' variable has the same name as the new parameter, which
is not legal in C99 and normally triggers compiler warning or error.


> > > -int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required)
> > > +int tap_open(char *ifname, int ifname_size, char *dev, int dev_size,
> > 
> > dev is never modified, so it should be const char *.
> 
> ok.
> 
> > > +			int *vnet_hdr, int vnet_hdr_required)
> > >  {
> > >      struct ifreq ifr;
> > >      int fd, ret;
> > > +    const char *path;
> > >  
> > > -    TFR(fd = open("/dev/net/tun", O_RDWR));
> > > +    if (dev[0] == '\0')
> > 
> > == 0 checks are ugly. if (*dev) is shorter, and it's a standard C
> > idiom to detect non-empty string. Or better pass NULL if no device,
> > then you can just path = dev ? dev : "/dev/net/tun".
> 
> Agreed in principle, but I was following the style that is already used
> in the same function, and I think consistency is more important in
> this case.

qemu code in general is inconsistent. Let's make new code look sane,
over time most of it will get converted.

> > > +	path = "/dev/net/tun";
> > > +    else
> > > +	path = dev;
> > 
> > Please do not indent by single space.
> 
> For some reason, this file uses four character indentation, which
> I followed for consistency. In the patch this gets mangled when
> some lines use only spaces for indentation and others use only
> tabs.
> 
> I could change to using only spaces for indentation if that's preferred.

Coding style says you should use spaces for indentation.

> > > diff --git a/net/tap.c b/net/tap.c
> > > index 0d8b424..14ddf65 100644
> > > --- a/net/tap.c
> > > +++ b/net/tap.c
> > > @@ -343,12 +343,17 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
> > >  {
> > >      int fd, vnet_hdr_required;
> > >      char ifname[128] = {0,};
> > > +    char dev[128] = {0,};
> > >      const char *setup_script;
> > >  
> > >      if (qemu_opt_get(opts, "ifname")) {
> > >          pstrcpy(ifname, sizeof(ifname), qemu_opt_get(opts, "ifname"));
> > >      }
> > >  
> > > +    if (qemu_opt_get(opts, "dev")) {
> > > +        pstrcpy(dev, sizeof(dev), qemu_opt_get(opts, "dev"));
> > > +    }
> > > +
> > 
> > While in this case this is unlikely to be a problem in practice, we still
> > should not add arbitrary limitations on file name lengths.  Just teach
> > tap_open to get NULL instead of and empty string, or better supply
> > default /dev/net/tun to the option, and you will not need the strcpy.
> 
> Right, I will do that, or alternatively make dev an input/output argument,
> see below.

input/output will require allocating storage for it,
so it's more work (assuming length of 128 characters is evil).
Not sure it's a good idea.

> > >          snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> > > -                 "ifname=%s,script=%s,downscript=%s",
> > > -                 ifname, script, downscript);
> > > +                 "ifname=%s,dev=%s,script=%s,downscript=%s",
> > 
> > This will look strange if dev is not supplied, will it not?
> > Also, I wonder whether there might be any scripts parsing
> > info string from monitor, that will get broken by the
> > extra parameter. How about we only print dev if it
> > is not the default?
> 
> Right. I originally planned to return "/dev/net/tun" from tap_open
> in that case, but I forgot to put that in. It would also not work well
> for Solaris and BSD unless I add untested code there.

I think it's better to add untested feature than intentionally
skip it. It's easier for people familiar with the platform to
fix a broken feature than to guess what feature needs to be filled in.

If you don't you, should replace ifndef WINDOWS by ifdef LINUX or
something like that.

> I guess it would be consistent to do that, but unless someone insists
> on it, I'll just follow your advice here and remove it from being printed.
> 
> > > +    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name][,dev=str][,script=file]\n"
> > > +    "        [,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off]\n"
> > >      "                connect the host TAP network interface to VLAN 'n' and use the\n"
> > >      "                network scripts 'file' (default=%s)\n"
> > >      "                and 'dfile' (default=%s);\n"
> > >      "                use '[down]script=no' to disable script execution;\n"
> > >      "                use 'fd=h' to connect to an already opened TAP interface\n"
> > > +    "                use 'dev=str' to open a specific tap character device\n"
> > 
> > please document default /dev/net/tun
> 
> ok.
> 
> Thanks for the review!
> 
> 	Arnd