Re: [Qemu-devel] [PATCH] tap-linux: report an error for invalid sndbuf

[Amos Kong]

On Sun, Apr 27, 2014 at 05:20:28PM +0300, Michael S. Tsirkin wrote:
> On Sun, Apr 27, 2014 at 10:09:10PM +0800, Amos Kong wrote:
> > We use default sndbuf (INT_MAX) if user assigns an invalid sndbuf.
> > This patch just added an error note.
> > 
> > Signed-off-by: Amos Kong <address@hidden>
> > ---
> >  net/tap-linux.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/tap-linux.c b/net/tap-linux.c
> > index 812bf2d..fc0cc0d 100644
> > --- a/net/tap-linux.c
> > +++ b/net/tap-linux.c
> > @@ -130,9 +130,16 @@ int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
> >  {
> >      int sndbuf;
> >  
> > -    sndbuf = !tap->has_sndbuf       ? TAP_DEFAULT_SNDBUF :
> > -             tap->sndbuf > INT_MAX  ? INT_MAX :
> > -             tap->sndbuf;
> > +    if (!tap->has_sndbuf) {
> > +        sndbuf = TAP_DEFAULT_SNDBUF;
> > +    } else if (tap->sndbuf > INT_MAX) {
> > +        sndbuf = TAP_DEFAULT_SNDBUF;
> > +        error_report("given sndbuf isn't an integer, "
> > +                     "or it's larger than INT_MAX(%d). "
> > +                     "use default sndbuf(%d)", INT_MAX, INT_MAX);
> 
> I think that converting a huge value to INT_MAX is
> actually more reasonable.

Yes, this is the current behavir.


Current TAP_DEFAULT_SNDBUF is zero ....

> For example, comment in json schema says:
> # @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes.
> If someone sets it to 1T why not make it work?

> Failing if it's not an integer sounds like a good feature.


I posted another patch to fix qapi/opts-visitor.c
Then too huge value will be catched in that place, and report an error.

I tested with sndbuf=8388607T, it can pass fixed qapi checking, but it
will be converted to INT_MAX in tap_set_sndbuf().


We can extend the buf size by changing the type of 'sndbuf' from 'int'
to 'int64_t'. the type of skbbuf in kernel also need to be updated.
I will have a try.
 
> > +    } else {
> > +        sndbuf = tap->sndbuf;
> > +    }
> >  
> >      if (!sndbuf) {
> >          sndbuf = INT_MAX;

... zero sndbuf will be converted to INT_MAX here.

-- 
                        Amos.