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

[Arnd Bergmann <arnd@arndb.de>]

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. 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.
 
> > -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.
 
> > +	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.

> > 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.

> >          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 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