[4/4] aplay: Limit VUMeter progress bar to 100 for negative as well
diff mbox series

Message ID 20191120042856.415854-4-rosenp@gmail.com
State New
Headers show
Series
  • [1/4] treewide: sys/poll to poll
Related show

Commit Message

Rosen Penev Nov. 20, 2019, 4:28 a.m. UTC
If the progress bar somehow becomes negative, it ends up overwritting
tmp. Fixes this GCC warning:

aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes
 into a region of size 4 [-Wformat-overflow=]
 1747 |    sprintf(tmp, "%02d%%", maxperc[c]);

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 aplay/aplay.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Takashi Iwai Nov. 20, 2019, 6:26 a.m. UTC | #1
On Wed, 20 Nov 2019 05:28:56 +0100,
Rosen Penev wrote:
> 
> If the progress bar somehow becomes negative, it ends up overwritting
> tmp. Fixes this GCC warning:
> 
> aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes
>  into a region of size 4 [-Wformat-overflow=]
>  1747 |    sprintf(tmp, "%02d%%", maxperc[c]);

This is false-positive.  The value passed there is guaranteed to be a 
positive integer at the calculation time.


thanks,

Takashi

> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  aplay/aplay.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/aplay/aplay.c b/aplay/aplay.c
> index 72fa567..9c5a11b 100644
> --- a/aplay/aplay.c
> +++ b/aplay/aplay.c
> @@ -54,6 +54,8 @@
>  #include "formats.h"
>  #include "version.h"
>  
> +#define ABS(a)  (a) < 0 ? -(a) : (a)
> +
>  #ifdef SND_CHMAP_API_VERSION
>  #define CONFIG_SUPPORT_CHMAP	1
>  #endif
> @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc)
>  			line[bar_length + 6 + 1 + p] = '+';
>  		else
>  			line[bar_length - p - 1] = '+';
> -		if (maxperc[c] > 99)
> +		if (ABS(maxperc[c]) > 99)
>  			sprintf(tmp, "MAX");
>  		else
>  			sprintf(tmp, "%02d%%", maxperc[c]);
> -- 
> 2.23.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Rosen Penev Nov. 20, 2019, 6:36 a.m. UTC | #2
On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Wed, 20 Nov 2019 05:28:56 +0100,
> Rosen Penev wrote:
> >
> > If the progress bar somehow becomes negative, it ends up overwritting
> > tmp. Fixes this GCC warning:
> >
> > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes
> >  into a region of size 4 [-Wformat-overflow=]
> >  1747 |    sprintf(tmp, "%02d%%", maxperc[c]);
>
> This is false-positive.  The value passed there is guaranteed to be a
> positive integer at the calculation time.
Sure. But best to silence GCC. It probably optimizes better this way.
>
>
> thanks,
>
> Takashi
>
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  aplay/aplay.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/aplay/aplay.c b/aplay/aplay.c
> > index 72fa567..9c5a11b 100644
> > --- a/aplay/aplay.c
> > +++ b/aplay/aplay.c
> > @@ -54,6 +54,8 @@
> >  #include "formats.h"
> >  #include "version.h"
> >
> > +#define ABS(a)  (a) < 0 ? -(a) : (a)
> > +
> >  #ifdef SND_CHMAP_API_VERSION
> >  #define CONFIG_SUPPORT_CHMAP 1
> >  #endif
> > @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc)
> >                       line[bar_length + 6 + 1 + p] = '+';
> >               else
> >                       line[bar_length - p - 1] = '+';
> > -             if (maxperc[c] > 99)
> > +             if (ABS(maxperc[c]) > 99)
> >                       sprintf(tmp, "MAX");
> >               else
> >                       sprintf(tmp, "%02d%%", maxperc[c]);
> > --
> > 2.23.0
> >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >
Takashi Iwai Nov. 20, 2019, 6:48 a.m. UTC | #3
On Wed, 20 Nov 2019 07:36:19 +0100,
Rosen Penev wrote:
> 
> On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Wed, 20 Nov 2019 05:28:56 +0100,
> > Rosen Penev wrote:
> > >
> > > If the progress bar somehow becomes negative, it ends up overwritting
> > > tmp. Fixes this GCC warning:
> > >
> > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes
> > >  into a region of size 4 [-Wformat-overflow=]
> > >  1747 |    sprintf(tmp, "%02d%%", maxperc[c]);
> >
> > This is false-positive.  The value passed there is guaranteed to be a
> > positive integer at the calculation time.
> Sure. But best to silence GCC. It probably optimizes better this way.

I guess this adds more code in binary.  Comparing with 99U would work?


Takashi


> >
> >
> > thanks,
> >
> > Takashi
> >
> > >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  aplay/aplay.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/aplay/aplay.c b/aplay/aplay.c
> > > index 72fa567..9c5a11b 100644
> > > --- a/aplay/aplay.c
> > > +++ b/aplay/aplay.c
> > > @@ -54,6 +54,8 @@
> > >  #include "formats.h"
> > >  #include "version.h"
> > >
> > > +#define ABS(a)  (a) < 0 ? -(a) : (a)
> > > +
> > >  #ifdef SND_CHMAP_API_VERSION
> > >  #define CONFIG_SUPPORT_CHMAP 1
> > >  #endif
> > > @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc)
> > >                       line[bar_length + 6 + 1 + p] = '+';
> > >               else
> > >                       line[bar_length - p - 1] = '+';
> > > -             if (maxperc[c] > 99)
> > > +             if (ABS(maxperc[c]) > 99)
> > >                       sprintf(tmp, "MAX");
> > >               else
> > >                       sprintf(tmp, "%02d%%", maxperc[c]);
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > Alsa-devel mailing list
> > > Alsa-devel@alsa-project.org
> > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > >
>
Rosen Penev Nov. 20, 2019, 5:55 p.m. UTC | #4
On Tue, Nov 19, 2019 at 10:48 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Wed, 20 Nov 2019 07:36:19 +0100,
> Rosen Penev wrote:
> >
> > On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > On Wed, 20 Nov 2019 05:28:56 +0100,
> > > Rosen Penev wrote:
> > > >
> > > > If the progress bar somehow becomes negative, it ends up overwritting
> > > > tmp. Fixes this GCC warning:
> > > >
> > > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes
> > > >  into a region of size 4 [-Wformat-overflow=]
> > > >  1747 |    sprintf(tmp, "%02d%%", maxperc[c]);
> > >
> > > This is false-positive.  The value passed there is guaranteed to be a
> > > positive integer at the calculation time.
> > Sure. But best to silence GCC. It probably optimizes better this way.
>
> I guess this adds more code in binary.  Comparing with 99U would work?
I tried that. Here are some results:

134348 for this patch
134832 if changed to U. Also tried casting lhs to unnsigned int, same size.
135125 originally

I understand this is an exercise in micro-optimization, but still.

Sizes are in bytes. It's the size of a compressed OpenWrt archive.

>
>
> Takashi
>
>
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > >
> > > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > > ---
> > > >  aplay/aplay.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/aplay/aplay.c b/aplay/aplay.c
> > > > index 72fa567..9c5a11b 100644
> > > > --- a/aplay/aplay.c
> > > > +++ b/aplay/aplay.c
> > > > @@ -54,6 +54,8 @@
> > > >  #include "formats.h"
> > > >  #include "version.h"
> > > >
> > > > +#define ABS(a)  (a) < 0 ? -(a) : (a)
> > > > +
> > > >  #ifdef SND_CHMAP_API_VERSION
> > > >  #define CONFIG_SUPPORT_CHMAP 1
> > > >  #endif
> > > > @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc)
> > > >                       line[bar_length + 6 + 1 + p] = '+';
> > > >               else
> > > >                       line[bar_length - p - 1] = '+';
> > > > -             if (maxperc[c] > 99)
> > > > +             if (ABS(maxperc[c]) > 99)
> > > >                       sprintf(tmp, "MAX");
> > > >               else
> > > >                       sprintf(tmp, "%02d%%", maxperc[c]);
> > > > --
> > > > 2.23.0
> > > >
> > > > _______________________________________________
> > > > Alsa-devel mailing list
> > > > Alsa-devel@alsa-project.org
> > > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > > >
> >
Rosen Penev Nov. 20, 2019, 7:12 p.m. UTC | #5
On Wed, Nov 20, 2019 at 10:34 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Wed, 20 Nov 2019 18:55:43 +0100,
> Rosen Penev wrote:
> >
> > On Tue, Nov 19, 2019 at 10:48 PM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > On Wed, 20 Nov 2019 07:36:19 +0100,
> > > Rosen Penev wrote:
> > > >
> > > > On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > > >
> > > > > On Wed, 20 Nov 2019 05:28:56 +0100,
> > > > > Rosen Penev wrote:
> > > > > >
> > > > > > If the progress bar somehow becomes negative, it ends up overwritting
> > > > > > tmp. Fixes this GCC warning:
> > > > > >
> > > > > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes
> > > > > >  into a region of size 4 [-Wformat-overflow=]
> > > > > >  1747 |    sprintf(tmp, "%02d%%", maxperc[c]);
> > > > >
> > > > > This is false-positive.  The value passed there is guaranteed to be a
> > > > > positive integer at the calculation time.
> > > > Sure. But best to silence GCC. It probably optimizes better this way.
> > >
> > > I guess this adds more code in binary.  Comparing with 99U would work?
> > I tried that. Here are some results:
> >
> > 134348 for this patch
> > 134832 if changed to U. Also tried casting lhs to unnsigned int, same size.
> > 135125 originally
> >
> > I understand this is an exercise in micro-optimization, but still.
> >
> > Sizes are in bytes. It's the size of a compressed OpenWrt archive.
>
> Thanks for the analysis.  It's surprising, though, the original code
> became bigger.
I've learned not to question the compiler. If it complains, it means
it generates suboptimal code.
>
> The cast of lhs is basically superfluous since C performs the cast
> implicitly at comparison, BTW.
>
> In anyway, the number tells.  Could you respin this patch?
I can resend. Not sure what you really want.
> Meanwhile I'll apply the rest patches.
>
>
> thanks,
>
> Takashi

Patch
diff mbox series

diff --git a/aplay/aplay.c b/aplay/aplay.c
index 72fa567..9c5a11b 100644
--- a/aplay/aplay.c
+++ b/aplay/aplay.c
@@ -54,6 +54,8 @@ 
 #include "formats.h"
 #include "version.h"
 
+#define ABS(a)  (a) < 0 ? -(a) : (a)
+
 #ifdef SND_CHMAP_API_VERSION
 #define CONFIG_SUPPORT_CHMAP	1
 #endif
@@ -1741,7 +1743,7 @@  static void print_vu_meter_stereo(int *perc, int *maxperc)
 			line[bar_length + 6 + 1 + p] = '+';
 		else
 			line[bar_length - p - 1] = '+';
-		if (maxperc[c] > 99)
+		if (ABS(maxperc[c]) > 99)
 			sprintf(tmp, "MAX");
 		else
 			sprintf(tmp, "%02d%%", maxperc[c]);