[PATCH 0/2] hw/s390x/ccw: Cleanup basename() and dirname()

[PATCH 0/2] hw/s390x/ccw: Cleanup basename() and dirname()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

As commit 3e015d815b3f ("use g_path_get_basename instead of basename")
said, g_path_get_basename() and g_path_get_dirname() should be preferred
over basename() and dirname(), since g_path_get_basename() and
g_path_get_dirname() are portable utility functions that have the
advantage of not modifing the string argument.

But commit 3e015d815b3f missed a use of dirname() and basename() in
hw/s390x/ccw.

And basename() (in vfio/container) caused compile breakage with the Musl
C library [1].

To avoid similar breakage and improve portability, replace basename()
and dirname() with g_path_get_basename() and g_path_get_dirname().

[1]: https://lore.kernel.org/all/20231212010228.2701544-1-raj.khem@gmail.com/

---
Zhao Liu (2):
  hw/s390x/ccw: Replace basename() with g_path_get_basename()
  hw/s390x/ccw: Replace dirname() with g_path_get_dirname()

 hw/s390x/s390-ccw.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/s390x/ccw: Replace basename() with g_path_get_basename()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

g_path_get_basename() is a portable utility function that has the
advantage of not modifing the string argument, so it should be
preferred over basename().

And also to avoid potential compile breakage with the Musl C library
similar to [1], replace basename() with g_path_get_basename().

[1]: https://lore.kernel.org/all/20231212010228.2701544-1-raj.khem@gmail.com/

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credit:
  Cédric: Referred his description of similar cleanup in vfio/container.
---
 hw/s390x/s390-ccw.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index e2d86d96e728..ab7022a3abe8 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -76,7 +76,8 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
                                   Error **errp)
 {
     unsigned int cssid, ssid, devid;
-    char dev_path[PATH_MAX] = {0}, *tmp;
+    char dev_path[PATH_MAX] = {0};
+    g_autofree char *tmp = NULL;
 
     if (!sysfsdev) {
         error_setg(errp, "No host device provided");
@@ -92,7 +93,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
 
     cdev->mdevid = g_path_get_basename(dev_path);
 
-    tmp = basename(dirname(dev_path));
+    tmp = g_path_get_basename(dirname(dev_path));
     if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) {
         error_setg_errno(errp, errno, "Failed to read %s", tmp);
         return;
-- 
2.34.1

[PATCH 2/2] hw/s390x/ccw: Replace dirname() with g_path_get_dirname()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

As commit 3e015d815b3f ("use g_path_get_basename instead of basename")
said, g_path_get_dirname() should be preferred over dirname() since
the former is a portable utility function that has the advantage of not
modifing the string argument.

Replace dirname() with g_path_get_dirname().

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credit:
  Cédric: Referred his words for g_path_get_basename() and
          g_path_get_dirname() has the same advantage.
---
 hw/s390x/s390-ccw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index ab7022a3abe8..5261e66724f1 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -77,6 +77,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
 {
     unsigned int cssid, ssid, devid;
     char dev_path[PATH_MAX] = {0};
+    g_autofree char *tmp_dir = NULL;
     g_autofree char *tmp = NULL;
 
     if (!sysfsdev) {
@@ -93,7 +94,8 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
 
     cdev->mdevid = g_path_get_basename(dev_path);
 
-    tmp = g_path_get_basename(dirname(dev_path));
+    tmp_dir = g_path_get_dirname(dev_path);
+    tmp = g_path_get_basename(tmp_dir);
     if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) {
         error_setg_errno(errp, errno, "Failed to read %s", tmp);
         return;
-- 
2.34.1

Re: [PATCH 0/2] hw/s390x/ccw: Cleanup basename() and dirname()

[Eric Farman]

On Fri, 2023-12-22 at 01:19 +0800, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> As commit 3e015d815b3f ("use g_path_get_basename instead of
> basename")
> said, g_path_get_basename() and g_path_get_dirname() should be
> preferred
> over basename() and dirname(), since g_path_get_basename() and
> g_path_get_dirname() are portable utility functions that have the
> advantage of not modifing the string argument.
> 
> But commit 3e015d815b3f missed a use of dirname() and basename() in
> hw/s390x/ccw.
> 
> And basename() (in vfio/container) caused compile breakage with the
> Musl
> C library [1].
> 
> To avoid similar breakage and improve portability, replace basename()
> and dirname() with g_path_get_basename() and g_path_get_dirname().
> 
> [1]:
> https://lore.kernel.org/all/20231212010228.2701544-1-raj.khem@gmail.com/
> 
> ---
> Zhao Liu (2):
>   hw/s390x/ccw: Replace basename() with g_path_get_basename()
>   hw/s390x/ccw: Replace dirname() with g_path_get_dirname()
> 
>  hw/s390x/s390-ccw.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH 0/2] hw/s390x/ccw: Cleanup basename() and dirname()

[Zhao Liu]

On Thu, Dec 21, 2023 at 04:29:31PM -0500, Eric Farman wrote:
> Date: Thu, 21 Dec 2023 16:29:31 -0500
> From: Eric Farman <farman@linux.ibm.com>
> Subject: Re: [PATCH 0/2] hw/s390x/ccw: Cleanup basename() and dirname()
> 
> On Fri, 2023-12-22 at 01:19 +0800, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > As commit 3e015d815b3f ("use g_path_get_basename instead of
> > basename")
> > said, g_path_get_basename() and g_path_get_dirname() should be
> > preferred
> > over basename() and dirname(), since g_path_get_basename() and
> > g_path_get_dirname() are portable utility functions that have the
> > advantage of not modifing the string argument.
> > 
> > But commit 3e015d815b3f missed a use of dirname() and basename() in
> > hw/s390x/ccw.
> > 
> > And basename() (in vfio/container) caused compile breakage with the
> > Musl
> > C library [1].
> > 
> > To avoid similar breakage and improve portability, replace basename()
> > and dirname() with g_path_get_basename() and g_path_get_dirname().
> > 
> > [1]:
> > https://lore.kernel.org/all/20231212010228.2701544-1-raj.khem@gmail.com/
> > 
> > ---
> > Zhao Liu (2):
> >   hw/s390x/ccw: Replace basename() with g_path_get_basename()
> >   hw/s390x/ccw: Replace dirname() with g_path_get_dirname()
> > 
> >  hw/s390x/s390-ccw.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> 
> Reviewed-by: Eric Farman <farman@linux.ibm.com>

Thanks Eric!

-Zhao

Re: [PATCH 1/2] hw/s390x/ccw: Replace basename() with g_path_get_basename()

[Cédric Le Goater]

On 12/21/23 18:19, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> g_path_get_basename() is a portable utility function that has the
> advantage of not modifing the string argument, so it should be
> preferred over basename().
> 
> And also to avoid potential compile breakage with the Musl C library
> similar to [1], replace basename() with g_path_get_basename().
> 
> [1]: https://lore.kernel.org/all/20231212010228.2701544-1-raj.khem@gmail.com/
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
> Suggested by credit:
>    Cédric: Referred his description of similar cleanup in vfio/container.
> ---
>   hw/s390x/s390-ccw.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index e2d86d96e728..ab7022a3abe8 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -76,7 +76,8 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>                                     Error **errp)
>   {
>       unsigned int cssid, ssid, devid;
> -    char dev_path[PATH_MAX] = {0}, *tmp;
> +    char dev_path[PATH_MAX] = {0};
> +    g_autofree char *tmp = NULL;
>   
>       if (!sysfsdev) {
>           error_setg(errp, "No host device provided");
> @@ -92,7 +93,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>   
>       cdev->mdevid = g_path_get_basename(dev_path);
>   
> -    tmp = basename(dirname(dev_path));
> +    tmp = g_path_get_basename(dirname(dev_path));
>       if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) {
>           error_setg_errno(errp, errno, "Failed to read %s", tmp);
>           return;

Re: [PATCH 2/2] hw/s390x/ccw: Replace dirname() with g_path_get_dirname()

[Cédric Le Goater]

On 12/21/23 18:19, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> As commit 3e015d815b3f ("use g_path_get_basename instead of basename")
> said, g_path_get_dirname() should be preferred over dirname() since
> the former is a portable utility function that has the advantage of not
> modifing the string argument.
> 
> Replace dirname() with g_path_get_dirname().
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
> Suggested by credit:
>    Cédric: Referred his words for g_path_get_basename() and
>            g_path_get_dirname() has the same advantage.
> ---
>   hw/s390x/s390-ccw.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index ab7022a3abe8..5261e66724f1 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -77,6 +77,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>   {
>       unsigned int cssid, ssid, devid;
>       char dev_path[PATH_MAX] = {0};
> +    g_autofree char *tmp_dir = NULL;
>       g_autofree char *tmp = NULL;
>   
>       if (!sysfsdev) {
> @@ -93,7 +94,8 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>   
>       cdev->mdevid = g_path_get_basename(dev_path);
>   
> -    tmp = g_path_get_basename(dirname(dev_path));
> +    tmp_dir = g_path_get_dirname(dev_path);
> +    tmp = g_path_get_basename(tmp_dir);
>       if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) {
>           error_setg_errno(errp, errno, "Failed to read %s", tmp);
>           return;