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
