[PATCH 0/2] tools/libs,xen/tools: remove dead code

[PATCH 0/2] tools/libs,xen/tools: remove dead code

[Ariel Otilibili]

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

[PATCH 1/2] tools/libs: remove dead code

[Ariel Otilibili]

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.47.1

[PATCH 2/2] xen/tools: remove dead code

[Ariel Otilibili]

Default switch case exits directly; these instructions are never reached.

The file was taken from Linux; the copies have diverged a lot since:

```
$ diff -u xen/xen/tools/kconfig/expr.c linux/scripts/kconfig/expr.c | wc -l
984
```

Therefore the change is only applied locally.

Coverity-ID: 1458052
Fixes: 8c271b7584 ("build: import Kbuild/Kconfig from Linux 4.3")
Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr>
--
Cc: Doug Goldstein <cardoe@cardoe.com>
---
 xen/tools/kconfig/expr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/tools/kconfig/expr.c b/xen/tools/kconfig/expr.c
index 77ffff3a05..be1400cdaf 100644
--- a/xen/tools/kconfig/expr.c
+++ b/xen/tools/kconfig/expr.c
@@ -1125,8 +1125,6 @@ static int expr_compare_type(enum expr_type t1, enum expr_type t2)
 	default:
 		return -1;
 	}
-	printf("[%dgt%d?]", t1, t2);
-	return 0;
 }
 
 void expr_print(struct expr *e,
-- 
2.47.1

Re: [PATCH 2/2] xen/tools: remove dead code

[Jan Beulich]

On 20.12.2024 17:55, Ariel Otilibili wrote:
> Default switch case exits directly; these instructions are never reached.
> 
> The file was taken from Linux; the copies have diverged a lot since:
> 
> ```
> $ diff -u xen/xen/tools/kconfig/expr.c linux/scripts/kconfig/expr.c | wc -l
> 984
> ```
> 
> Therefore the change is only applied locally.
> 
> Coverity-ID: 1458052
> Fixes: 8c271b7584 ("build: import Kbuild/Kconfig from Linux 4.3")
> Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr>

As the title of the commit referenced already clarifies - this is once
again code we took from elsewhere, so imo wants updating at the origin
first. Then import that commit into Xen.

Jan

Re: [PATCH 2/2] xen/tools: remove dead code

[Ariel Otilibili-Anieli]

On Tuesday, December 24, 2024 09:43 CET, Jan Beulich <jbeulich@suse.com> wrote:

> On 20.12.2024 17:55, Ariel Otilibili wrote:
> > Default switch case exits directly; these instructions are never reached.
> > 
> > The file was taken from Linux; the copies have diverged a lot since:
> > 
> > ```
> > $ diff -u xen/xen/tools/kconfig/expr.c linux/scripts/kconfig/expr.c | wc -l
> > 984
> > ```
> > 
> > Therefore the change is only applied locally.
> > 
> > Coverity-ID: 1458052
> > Fixes: 8c271b7584 ("build: import Kbuild/Kconfig from Linux 4.3")
> > Signed-off-by: Ariel Otilibili <Ariel.Otilibili-Anieli@eurecom.fr>
> 
> As the title of the commit referenced already clarifies - this is once
> again code we took from elsewhere, so imo wants updating at the origin
> first. Then import that commit into Xen.

Indeed, Jan; thanks for the feedback. I am pushing a new series for this hunk; it was fixed in Linux.
I think in commits 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)
```
> 
> Jan

[PATCH v2 0/2] tools/libs,xen/tools: remove dead code

[Ariel Otilibili]

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

[PATCH v2 1/2] tools/libs: remove dead code

[Ariel Otilibili]

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.0

Re: [PATCH v2 1/2] tools/libs: remove dead code

[Anthony PERARD]

Hi 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

[PATCH v2 2/2] xen/tools: remove dead code

[Ariel Otilibili]

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.0

Re: [PATCH v2 2/2] xen/tools: remove dead code

[Jan Beulich]

On 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