tools/libs/light/libxl_create.c | 1 - tools/libs/light/libxl_device.c | 1 - xen/tools/kconfig/expr.c | 2 -- 3 files changed, 4 deletions(-)
Hello, This series addresses the Coverity IDs 1056148, 1056149, & 1458052. Thank you, Ariel Otilibili (2): tools/libs: remove dead code xen/tools: remove dead code tools/libs/light/libxl_create.c | 1 - tools/libs/light/libxl_device.c | 1 - xen/tools/kconfig/expr.c | 2 -- 3 files changed, 4 deletions(-) -- 2.47.1
Hello, This series addresses the Coverity IDs 1056148, 1056149, & 1458052. Thank you, --- v2: * addresse feedback of Jan Beulich on expr.c Ariel Otilibili (2): tools/libs: remove dead code xen/tools: remove dead code tools/libs/light/libxl_create.c | 1 - tools/libs/light/libxl_device.c | 1 - xen/tools/kconfig/expr.c | 13 +++++-------- 3 files changed, 5 insertions(+), 10 deletions(-) -- 2.43.0
Default switch cases skip these steps; these instructions are never reached.
Coverity-IDs: 1056148, 1056149
Fixes: 0a69ea908d ("libxl: ao: convert libxl__spawn_*")
Fixes: 643b106b40 ("libxl: do not use tap disk backend other than for raw and vhd")
Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr>
--
Cc: Anthony PERARD <anthony.perard@vates.tech>
Cc: Juergen Gross <jgross@suse.com>
Cc: "Daniel P. Smith" <dpsmith@apertussolutions.com>
---
tools/libs/light/libxl_create.c | 1 -
tools/libs/light/libxl_device.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index e03599ea99..d0271bef7e 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1890,7 +1890,6 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
ret = ERROR_INVAL;
goto error_out;
}
- abort(); /* not reached */
error_out:
assert(ret);
diff --git a/tools/libs/light/libxl_device.c b/tools/libs/light/libxl_device.c
index 4faa5fa3bd..96046803e1 100644
--- a/tools/libs/light/libxl_device.c
+++ b/tools/libs/light/libxl_device.c
@@ -392,7 +392,6 @@ static int disk_try_backend(disk_try_backend_args *a,
return 0;
}
- abort(); /* notreached */
bad_format:
LOG(DEBUG, "Disk vdev=%s, backend %s unsuitable due to format %s",
--
2.43.0Hi Ariel,
On Tue, Dec 24, 2024 at 08:13:54PM +0100, Ariel Otilibili wrote:
> Default switch cases skip these steps; these instructions are never reached.
The "default" case might skip these statements, but the intention behind
those statements is to make sure that every other cases also skip these,
with "return" or "goto".
There's a comment on each of those statements, so it should be clear
enough that those are not expected to be executed. So I'd rather keep
those two statements.
But thanks.
> Coverity-IDs: 1056148, 1056149
> Fixes: 0a69ea908d ("libxl: ao: convert libxl__spawn_*")
> Fixes: 643b106b40 ("libxl: do not use tap disk backend other than for raw and vhd")
> Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr>
> ---
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index e03599ea99..d0271bef7e 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -1890,7 +1890,6 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
> ret = ERROR_INVAL;
> goto error_out;
> }
> - abort(); /* not reached */
>
> error_out:
> assert(ret);
> diff --git a/tools/libs/light/libxl_device.c b/tools/libs/light/libxl_device.c
> index 4faa5fa3bd..96046803e1 100644
> --- a/tools/libs/light/libxl_device.c
> +++ b/tools/libs/light/libxl_device.c
> @@ -392,7 +392,6 @@ static int disk_try_backend(disk_try_backend_args *a,
> return 0;
>
> }
> - abort(); /* notreached */
>
> bad_format:
> LOG(DEBUG, "Disk vdev=%s, backend %s unsuitable due to format %s",
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
The original file was imported from Linux; patched the entire
expr_compare_type() with the diff from Linux.
Commits wherein it might have been fixed in Linux:
- dfe8e56fc604 ("kconfig: add fallthrough comments to expr_compare_type()")
- 9ad86d747c46 ("kconfig: remove unreachable printf()").
```
$ diff -u xen/xen/tools/kconfig/expr.c linux/scripts/kconfig/expr.c | \
sed -ne '/expr_compare_type/,/return 0/{N;p}'
static int expr_compare_type(enum expr_type t1, enum expr_type t2)
{
if (t1 == t2)
@@ -1106,30 +999,27 @@
case E_GTH:
if (t2 == E_EQUAL || t2 == E_UNEQUAL)
return 1;
+ /* fallthrough */
case E_EQUAL:
case E_UNEQUAL:
if (t2 == E_NOT)
return 1;
+ /* fallthrough */
case E_NOT:
if (t2 == E_AND)
return 1;
+ /* fallthrough */
case E_AND:
if (t2 == E_OR)
return 1;
- case E_OR:
- if (t2 == E_LIST)
- return 1;
- case E_LIST:
- if (t2 == 0)
- return 1;
+ /* fallthrough */
default:
- return -1;
+ break;
}
- printf("[%dgt%d?]", t1, t2);
return 0;
}
$ cd linux/;
$ git log --oneline -1 --pretty='%h ("%s")'
8155b4ef3466 ("Add linux-next specific files for 20241220")
$ git remote -v
next git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git (fetch)
next git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git (push)
$ cd ../xen/
$ git log --oneline -1 --pretty='%h ("%s")'
6419020270 ("CHANGELOG: Mention LLC coloring feature on Arm")
$ git remote -v
up git://xenbits.xen.org/xen.git (fetch)
up git://xenbits.xen.org/xen.git (push)
```
Coverity-ID: 1458052
Fixes: 8c271b7584 ("build: import Kbuild/Kconfig from Linux 4.3")
Cc: Doug Goldstein <cardoe@cardoe.com>
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr>
---
xen/tools/kconfig/expr.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/xen/tools/kconfig/expr.c b/xen/tools/kconfig/expr.c
index 77ffff3a05..84af124626 100644
--- a/xen/tools/kconfig/expr.c
+++ b/xen/tools/kconfig/expr.c
@@ -1106,26 +1106,23 @@ static int expr_compare_type(enum expr_type t1, enum expr_type t2)
case E_GTH:
if (t2 == E_EQUAL || t2 == E_UNEQUAL)
return 1;
+ /* fallthrough */
case E_EQUAL:
case E_UNEQUAL:
if (t2 == E_NOT)
return 1;
+ /* fallthrough */
case E_NOT:
if (t2 == E_AND)
return 1;
+ /* fallthrough */
case E_AND:
if (t2 == E_OR)
return 1;
- case E_OR:
- if (t2 == E_LIST)
- return 1;
- case E_LIST:
- if (t2 == 0)
- return 1;
+ /* fallthrough */
default:
- return -1;
+ break;
}
- printf("[%dgt%d?]", t1, t2);
return 0;
}
--
2.43.0On 24.12.2024 20:13, Ariel Otilibili wrote:
> The original file was imported from Linux; patched the entire
> expr_compare_type() with the diff from Linux.
I'm afraid that it's quite likely that taking changes to just an isolated
function will not work very well. What's worse, ...
> Commits wherein it might have been fixed in Linux:
> - dfe8e56fc604 ("kconfig: add fallthrough comments to expr_compare_type()")
> - 9ad86d747c46 ("kconfig: remove unreachable printf()").
... these references to Linux commits then don't really help, as the isolated
changes may have different effects, and hence ...
> ```
> $ diff -u xen/xen/tools/kconfig/expr.c linux/scripts/kconfig/expr.c | \
> sed -ne '/expr_compare_type/,/return 0/{N;p}'
>
> static int expr_compare_type(enum expr_type t1, enum expr_type t2)
> {
> if (t1 == t2)
> @@ -1106,30 +999,27 @@
> case E_GTH:
> if (t2 == E_EQUAL || t2 == E_UNEQUAL)
> return 1;
> + /* fallthrough */
> case E_EQUAL:
> case E_UNEQUAL:
> if (t2 == E_NOT)
> return 1;
> + /* fallthrough */
> case E_NOT:
> if (t2 == E_AND)
> return 1;
> + /* fallthrough */
> case E_AND:
> if (t2 == E_OR)
> return 1;
> - case E_OR:
> - if (t2 == E_LIST)
> - return 1;
> - case E_LIST:
> - if (t2 == 0)
> - return 1;
> + /* fallthrough */
> default:
> - return -1;
> + break;
> }
> - printf("[%dgt%d?]", t1, t2);
> return 0;
> }
>
> $ cd linux/;
> $ git log --oneline -1 --pretty='%h ("%s")'
> 8155b4ef3466 ("Add linux-next specific files for 20241220")
>
> $ git remote -v
> next git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git (fetch)
> next git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git (push)
>
> $ cd ../xen/
> $ git log --oneline -1 --pretty='%h ("%s")'
> 6419020270 ("CHANGELOG: Mention LLC coloring feature on Arm")
>
> $ git remote -v
> up git://xenbits.xen.org/xen.git (fetch)
> up git://xenbits.xen.org/xen.git (push)
> ```
>
> Coverity-ID: 1458052
> Fixes: 8c271b7584 ("build: import Kbuild/Kconfig from Linux 4.3")
> Cc: Doug Goldstein <cardoe@cardoe.com>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr>
... an actual description of the (effect of the) changes done _here_ is missing.
For example, ....
> --- a/xen/tools/kconfig/expr.c
> +++ b/xen/tools/kconfig/expr.c
> @@ -1106,26 +1106,23 @@ static int expr_compare_type(enum expr_type t1, enum expr_type t2)
> case E_GTH:
> if (t2 == E_EQUAL || t2 == E_UNEQUAL)
> return 1;
> + /* fallthrough */
> case E_EQUAL:
> case E_UNEQUAL:
> if (t2 == E_NOT)
> return 1;
> + /* fallthrough */
> case E_NOT:
> if (t2 == E_AND)
> return 1;
> + /* fallthrough */
> case E_AND:
> if (t2 == E_OR)
> return 1;
> - case E_OR:
> - if (t2 == E_LIST)
> - return 1;
> - case E_LIST:
> - if (t2 == 0)
> - return 1;
> + /* fallthrough */
... it's unclear to me why removing handling of E_OR and E_LIST is actually correct.
All of this said - this looks like a wording issue: You did actually take two full
commits (adding in - see below - at least one change of your own). May I suggest
that you take those commits individually, retaining their titles and descriptions,
merely adding necessary further tags (Origin: and your own S-o-b)?
> default:
> - return -1;
> + break;
This change isn't part of either of the mentioned commits.
> }
> - printf("[%dgt%d?]", t1, t2);
> return 0;
> }
The "Suggested-by:" also isn't quite right imo. If anything what I suggested was
to take commits from Linux. But that's whole commits, not fragments thereof, nor
multiple of them folded (unless there's a good reason to do so). And for such
straight importing I don't think "Suggested-by:" would be quite applicable.
Jan
© 2016 - 2025 Red Hat, Inc.