| Message ID | 20241030140656.36540-1-abelova@astralinux.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: fix cases in switch in memory_dump | expand |
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 > > >
* 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 > > > > > >
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
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
> 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
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) {
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(-)