| Message ID | 1398607750-6305-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <akong@redhat.com> > --- > 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. 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. > + } else { > + sndbuf = tap->sndbuf; > + } > > if (!sndbuf) { > sndbuf = INT_MAX; > -- > 1.9.0
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 <akong@redhat.com> > > --- > > 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.
On Mon, Apr 28, 2014 at 01:03:04AM +0800, Amos Kong wrote: > 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 <akong@redhat.com> > > > --- > > > 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. The assumption always was that INT_MAX has same effect as infinity for tap. Do you really expect INT_MAX size to be exhausted by just one tap device? If we ever hit a configuration where that's not the case, I would say as the first step, let's make INT_MAX a practical infinity in kernel. I'd like to see a test case like this first though. > I will have a try. > > > > + } else { > > > + sndbuf = tap->sndbuf; > > > + } > > > > > > if (!sndbuf) { > > > sndbuf = INT_MAX; > > ... zero sndbuf will be converted to INT_MAX here. > > -- > Amos.
On Sun, Apr 27, 2014 at 08:16:58PM +0300, Michael S. Tsirkin wrote: > On Mon, Apr 28, 2014 at 01:03:04AM +0800, Amos Kong wrote: > > 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 <akong@redhat.com> > > > > --- > > > > 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. Hi Michael, > The assumption always was that INT_MAX has same effect as infinity for tap. > Do you really expect INT_MAX size to be exhausted by just one tap > device? > > If we ever hit a configuration where that's not the case, I would say > as the first step, let's make INT_MAX a practical infinity in kernel. > I'd like to see a test case like this first though. This patch [1] only added a hint if huge value that is larger than INT_MAX is changed to INT_MAX. We can't change it silently. The patch [2] fixed a qapi error, sndbuf accepts 'size' type (that is int64_t). [1] [PATCH] tap-linux: report an error for invalid [2] [PATCH] qapi: treat all negative return of strtosz_suffix() as error To adjust the max limitation is another issue, I think we can only adjust the limitation when we have real usecase (I don't have). Thanks, Amos > > I will have a try. > > > > > > + } else { > > > > + sndbuf = tap->sndbuf; > > > > + } > > > > > > > > if (!sndbuf) { > > > > sndbuf = INT_MAX; > > > > ... zero sndbuf will be converted to INT_MAX here.
On Mon, Apr 28, 2014 at 12:06:49PM +0800, Amos Kong wrote: > On Sun, Apr 27, 2014 at 08:16:58PM +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 28, 2014 at 01:03:04AM +0800, Amos Kong wrote: > > > 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 <akong@redhat.com> > > > > > --- > > > > > 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. > > Hi Michael, > > > The assumption always was that INT_MAX has same effect as infinity for tap. > > Do you really expect INT_MAX size to be exhausted by just one tap > > device? > > > > If we ever hit a configuration where that's not the case, I would say > > as the first step, let's make INT_MAX a practical infinity in kernel. > > I'd like to see a test case like this first though. > > > This patch [1] only added a hint if huge value that is larger than > INT_MAX is changed to INT_MAX. We can't change it silently. Why can't we? Seems safe to me. > The patch [2] fixed a qapi error, sndbuf accepts 'size' type (that is int64_t). > > [1] [PATCH] tap-linux: report an error for invalid > [2] [PATCH] qapi: treat all negative return of strtosz_suffix() as error 2 seems to make sense, will review. > > To adjust the max limitation is another issue, I think we can only > adjust the limitation when we have real usecase (I don't have). > > Thanks, Amos > > > > I will have a try. > > > > > > > > + } else { > > > > > + sndbuf = tap->sndbuf; > > > > > + } > > > > > > > > > > if (!sndbuf) { > > > > > sndbuf = INT_MAX; > > > > > > ... zero sndbuf will be converted to INT_MAX here.
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); + } else { + sndbuf = tap->sndbuf; + } if (!sndbuf) { sndbuf = INT_MAX;
We use default sndbuf (INT_MAX) if user assigns an invalid sndbuf. This patch just added an error note. Signed-off-by: Amos Kong <akong@redhat.com> --- net/tap-linux.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)