monitor: fix cases in switch in memory_dump

[PATCH] monitor: fix cases in switch in memory_dump

Posted by Anastasia Belova 1 year, 3 months ago

default case has no condition. So if it is placed
higher that other cases, they are unreachable.

Move dafult case down.

Found by Linux Verification Center (linuxtesting.org)

Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 monitor/hmp-cmds-target.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
index ff01cf9d8d..eea8ca047b 100644
--- a/monitor/hmp-cmds-target.c
+++ b/monitor/hmp-cmds-target.c
@@ -189,7 +189,6 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
         i = 0;
         while (i < l) {
             switch(wsize) {
-            default:
             case 1:
                 v = ldub_p(buf + i);
                 break;
@@ -202,6 +201,9 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
             case 8:
                 v = ldq_p(buf + i);
                 break;
+            default:
+                v = ldub_p(buf + i);
+                break;
             }
             monitor_printf(mon, " ");
             switch(format) {
-- 
2.47.0

Re: [PATCH] monitor: fix cases in switch in memory_dump

Posted by Phil Dennis-Jordan 1 year, 3 months ago

On Wed 30. Oct 2024 at 15:09, Anastasia Belova <abelova@astralinux.ru>
wrote:

> default case has no condition. So if it is placed
> higher that other cases, they are unreachable.
>
> Move dafult case down.
>

The stylistic merits might be debatable, but: the order of cases in a
switch block in C does not matter, the default case can appear anywhere.
The other cases are still reachable. So at minimum, the commit message is
incorrect.



> Found by Linux Verification Center (linuxtesting.org)
>
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>  monitor/hmp-cmds-target.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
> index ff01cf9d8d..eea8ca047b 100644
> --- a/monitor/hmp-cmds-target.c
> +++ b/monitor/hmp-cmds-target.c
> @@ -189,7 +189,6 @@ static void memory_dump(Monitor *mon, int count, int
> format, int wsize,
>          i = 0;
>          while (i < l) {
>              switch(wsize) {
> -            default:
>              case 1:
>                  v = ldub_p(buf + i);
>                  break;
> @@ -202,6 +201,9 @@ static void memory_dump(Monitor *mon, int count, int
> format, int wsize,
>              case 8:
>                  v = ldq_p(buf + i);
>                  break;
> +            default:
> +                v = ldub_p(buf + i);
> +                break;
>              }
>              monitor_printf(mon, " ");
>              switch(format) {
> --
> 2.47.0
>
>
>

Re: [PATCH] monitor: fix cases in switch in memory_dump

Posted by Anastasia Belova 1 year, 3 months ago


> 30 окт. 2024 г., в 22:03, Phil Dennis-Jordan <lists@philjordan.eu> написал(а):
> 
> 
> On Wed 30. Oct 2024 at 15:09, Anastasia Belova <abelova@astralinux.ru> wrote:
> default case has no condition. So if it is placed
> higher that other cases, they are unreachable.
> 
> Move dafult case down.
> 
> The stylistic merits might be debatable, but: the order of cases in a switch block in C does not matter, the default case can appear anywhere. The other cases are still reachable. So at minimum, the commit message is incorrect.
> 

You’re right, I didn’t know about this. Thank you for reply and patience!

Anastasia Belova

> Found by Linux Verification Center (linuxtesting.org)
> 
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
> monitor/hmp-cmds-target.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
> index ff01cf9d8d..eea8ca047b 100644
> --- a/monitor/hmp-cmds-target.c
> +++ b/monitor/hmp-cmds-target.c
> @@ -189,7 +189,6 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>       i = 0;
>       while (i < l) {
>           switch(wsize) {
> -            default:
>           case 1:
>               v = ldub_p(buf + i);
>               break;
> @@ -202,6 +201,9 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>           case 8:
>               v = ldq_p(buf + i);
>               break;
> +            default:
> +                v = ldub_p(buf + i);
> +                break;
>           }
>           monitor_printf(mon, " ");
>           switch(format) {
> -- 
> 2.47.0

Re: [PATCH] monitor: fix cases in switch in memory_dump

Posted by Dr. David Alan Gilbert 1 year, 3 months ago

* Phil Dennis-Jordan (lists@philjordan.eu) wrote:
> On Wed 30. Oct 2024 at 15:09, Anastasia Belova <abelova@astralinux.ru>
> wrote:
> 
> > default case has no condition. So if it is placed
> > higher that other cases, they are unreachable.
> >
> > Move dafult case down.
> >
> 
> The stylistic merits might be debatable, but: the order of cases in a
> switch block in C does not matter, the default case can appear anywhere.
> The other cases are still reachable. So at minimum, the commit message is
> incorrect.

I'd agree;  the analysis is wrong - it works as intended.
As for style, I'd normally agree that 'default' at end makes sense,
but:
  a) I hate duplicating code
  b) in a way this reads nicely:
                 default:
                 case 1:

      'default is the same as case 1'.

Dave

> 
> 
> > Found by Linux Verification Center (linuxtesting.org)
> >
> > Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> > ---
> >  monitor/hmp-cmds-target.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
> > index ff01cf9d8d..eea8ca047b 100644
> > --- a/monitor/hmp-cmds-target.c
> > +++ b/monitor/hmp-cmds-target.c
> > @@ -189,7 +189,6 @@ static void memory_dump(Monitor *mon, int count, int
> > format, int wsize,
> >          i = 0;
> >          while (i < l) {
> >              switch(wsize) {
> > -            default:
> >              case 1:
> >                  v = ldub_p(buf + i);
> >                  break;
> > @@ -202,6 +201,9 @@ static void memory_dump(Monitor *mon, int count, int
> > format, int wsize,
> >              case 8:
> >                  v = ldq_p(buf + i);
> >                  break;
> > +            default:
> > +                v = ldub_p(buf + i);
> > +                break;
> >              }
> >              monitor_printf(mon, " ");
> >              switch(format) {
> > --
> > 2.47.0
> >
> >
> >
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH] monitor: fix cases in switch in memory_dump

Posted by Peter Maydell 1 year, 3 months ago

On Thu, 31 Oct 2024 at 00:32, Dr. David Alan Gilbert <dave@treblig.org> wrote:
>
> * Phil Dennis-Jordan (lists@philjordan.eu) wrote:
> > On Wed 30. Oct 2024 at 15:09, Anastasia Belova <abelova@astralinux.ru>
> > wrote:
> >
> > > default case has no condition. So if it is placed
> > > higher that other cases, they are unreachable.
> > >
> > > Move dafult case down.
> > >
> >
> > The stylistic merits might be debatable, but: the order of cases in a
> > switch block in C does not matter, the default case can appear anywhere.
> > The other cases are still reachable. So at minimum, the commit message is
> > incorrect.
>
> I'd agree;  the analysis is wrong - it works as intended.
> As for style, I'd normally agree that 'default' at end makes sense,
> but:
>   a) I hate duplicating code
>   b) in a way this reads nicely:
>                  default:
>                  case 1:
>
>       'default is the same as case 1'.

Is it actually possible to get here with a wsize that
isn't 1,2,4 or 8, though? This function is used only
by the hmp 'x' and 'xp' commands. Those document that
the valid size specifications are b, h, w or g (for
8, 16, 32 or 64 bits), and monitor_parse_arguments()
doesn't seem to have any undocumented handling that
would result in a different size value. And the
code in memory_dump() doesn't do anything sensible
with a wsize other than 1, 2, 4 or 8 -- if you hand
it a wsize of 3, for instance, I think it will print
every third byte.

So I think that probably the default case here should
be g_assert_not_reached().

Disclaimer: I haven't played with the x and xp commands
to confirm that we really don't ever get here with a
funny wsize value.

thanks
-- PMM

Re: [PATCH] monitor: fix cases in switch in memory_dump

Posted by Peter Maydell 1 year, 3 months ago

On Thu, 31 Oct 2024 at 10:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 31 Oct 2024 at 00:32, Dr. David Alan Gilbert <dave@treblig.org> wrote:
> >
> > * Phil Dennis-Jordan (lists@philjordan.eu) wrote:
> > > On Wed 30. Oct 2024 at 15:09, Anastasia Belova <abelova@astralinux.ru>
> > > wrote:
> > >
> > > > default case has no condition. So if it is placed
> > > > higher that other cases, they are unreachable.
> > > >
> > > > Move dafult case down.
> > > >
> > >
> > > The stylistic merits might be debatable, but: the order of cases in a
> > > switch block in C does not matter, the default case can appear anywhere.
> > > The other cases are still reachable. So at minimum, the commit message is
> > > incorrect.
> >
> > I'd agree;  the analysis is wrong - it works as intended.
> > As for style, I'd normally agree that 'default' at end makes sense,
> > but:
> >   a) I hate duplicating code
> >   b) in a way this reads nicely:
> >                  default:
> >                  case 1:
> >
> >       'default is the same as case 1'.
>
> Is it actually possible to get here with a wsize that
> isn't 1,2,4 or 8, though? This function is used only
> by the hmp 'x' and 'xp' commands. Those document that
> the valid size specifications are b, h, w or g (for
> 8, 16, 32 or 64 bits), and monitor_parse_arguments()
> doesn't seem to have any undocumented handling that
> would result in a different size value. And the
> code in memory_dump() doesn't do anything sensible
> with a wsize other than 1, 2, 4 or 8 -- if you hand
> it a wsize of 3, for instance, I think it will print
> every third byte.
>
> So I think that probably the default case here should
> be g_assert_not_reached().

...better still, we could replace the whole switch(wsize)
with "v = ldn_p(buf + i, wsize);".

-- PMM