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