[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap,dev= option
From: |
Arnd Bergmann |
Subject: |
[Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap,dev= option |
Date: |
Wed, 9 Dec 2009 13:33:36 +0100 |
User-agent: |
KMail/1.12.2 (Linux/2.6.31-14-generic; KDE/4.3.2; x86_64; ; ) |
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
- [Qemu-devel] [PATCH, try 2] qemu/tap: add -net tap,dev= option, Arnd Bergmann, 2009/12/08
- Re: [Qemu-devel] [PATCH, try 2] qemu/tap: add -net tap,dev= option, Mark McLoughlin, 2009/12/09
- [Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap,dev= option, Michael S. Tsirkin, 2009/12/09
- [Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap,dev= option,
Arnd Bergmann <=
- Re: [Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap, dev= option, Christoph Egger, 2009/12/09
- [Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap,dev= option, Michael S. Tsirkin, 2009/12/09
- [Qemu-devel] [PATCH, try 2, version 2] qemu/tap: add -net tap, dev= option, Arnd Bergmann, 2009/12/09
- [Qemu-devel] Re: [PATCH, try 2, version 2] qemu/tap: add -net tap, dev= option, Michael S. Tsirkin, 2009/12/09
- Re: [Qemu-devel] Re: [PATCH, try 2, version 2] qemu/tap: add -net tap, dev= option, Arnd Bergmann, 2009/12/09
[Qemu-devel] Re: [PATCH, try 2] qemu/tap: add -net tap,dev= option, Anthony Liguori, 2009/12/09