1. Patchew
  2. Libvirt
  3. View series

[PATCH] network: fix check in virNetworkLoadConfig

Anastasia Belova posted 1 patch 4 months, 3 weeks ago
Failed in applying to current master (apply log)
src/conf/virnetworkobj.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] network: fix check in virNetworkLoadConfig
Posted by Anastasia Belova 4 months, 3 weeks ago 
virFileLinkPointsTo return non-zero value on fail. This value
may be positive or negative, so check should be != 0.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: bddbda99df ("network: Introduce virnetworkobj")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 src/conf/virnetworkobj.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index 20ee8eb58a..47658986e8 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -945,7 +945,7 @@ virNetworkLoadConfig(virNetworkObjList *nets,
     if ((autostartLink = virNetworkConfigFile(autostartDir, name)) == NULL)
         return NULL;
 
-    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
+    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) != 0)
         return NULL;
 
     if (!(def = virNetworkDefParse(NULL, configFile, xmlopt, false)))
-- 
2.30.2
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] network: fix check in virNetworkLoadConfig
Posted by Peter Krempa 4 months, 3 weeks ago 
On Wed, Dec 06, 2023 at 21:30:59 +0300, Anastasia Belova wrote:
> virFileLinkPointsTo return non-zero value on fail. This value
> may be positive or negative, so check should be != 0.

This statement is not true:

#define SAME_INODE(Stat_buf_1, Stat_buf_2) \
  ((Stat_buf_1).st_ino == (Stat_buf_2).st_ino \
   && (Stat_buf_1).st_dev == (Stat_buf_2).st_dev)

/* Return nonzero if checkLink and checkDest
 * refer to the same file.  Otherwise, return 0.
 */
int
virFileLinkPointsTo(const char *checkLink,
                    const char *checkDest)
{
    struct stat src_sb;
    struct stat dest_sb;

    return (stat(checkLink, &src_sb) == 0
            && stat(checkDest, &dest_sb) == 0
            && SAME_INODE(src_sb, dest_sb));
}

Based on the docs and the code virFileLinkPointsTo returns 'true'
typecast to int (thus non-zero) on success, if the ling (1st arg) points
to the second arg. Otherwise if the state can't be determined or the
linking doesn't exist it returns false, thus 0.

> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: bddbda99df ("network: Introduce virnetworkobj")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>  src/conf/virnetworkobj.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index 20ee8eb58a..47658986e8 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -945,7 +945,7 @@ virNetworkLoadConfig(virNetworkObjList *nets,
>      if ((autostartLink = virNetworkConfigFile(autostartDir, name)) == NULL)
>          return NULL;
>  
> -    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
> +    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) != 0)
>          return NULL;

Thus doing this would cause to always bail out as if it were failure if
the link points to the expected file.

In fact the original behaviour, which would be that the condition is
always false => dead code was correct, and the condition should be
removed.

'autostart' can be filled unconditionally.

In fact for a proper fix virFileLinkPointsTo should be also converted to
return bool.

>  
>      if (!(def = virNetworkDefParse(NULL, configFile, xmlopt, false)))
> -- 
> 2.30.2
> _______________________________________________
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-leave@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
[PATCH v2] network: convert virFileLinkPointsTo to return bool
Posted by Anastasia Belova 4 months, 3 weeks ago 
Convert prototype of virFileLinkPointsTo to return bool.
Remove dead checks in virDomainObjListLoadConfig and
virNetworkLoadConfig.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
v2: fix logic according to maintainer's answer
 src/conf/virdomainobjlist.c | 3 +--
 src/conf/virnetworkobj.c    | 3 +--
 src/util/virfile.c          | 2 +-
 src/util/virfile.h          | 2 +-
 4 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 0bd833257d..bb5807d00b 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -497,8 +497,7 @@ virDomainObjListLoadConfig(virDomainObjList *doms,
     if ((autostartLink = virDomainConfigFile(autostartDir, name)) == NULL)
         return NULL;
 
-    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
-        return NULL;
+    autostart = virFileLinkPointsTo(autostartLink, configFile);
 
     if (!(dom = virDomainObjListAddLocked(doms, &def, xmlopt, 0, &oldDef)))
         return NULL;
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index 20ee8eb58a..d5aa121e20 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -945,8 +945,7 @@ virNetworkLoadConfig(virNetworkObjList *nets,
     if ((autostartLink = virNetworkConfigFile(autostartDir, name)) == NULL)
         return NULL;
 
-    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
-        return NULL;
+    autostart = virFileLinkPointsTo(autostartLink, configFile);
 
     if (!(def = virNetworkDefParse(NULL, configFile, xmlopt, false)))
         return NULL;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 007b6cf512..f3108e99cf 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1697,7 +1697,7 @@ virFileWriteStr(const char *path, const char *str, mode_t mode)
 /* Return nonzero if checkLink and checkDest
  * refer to the same file.  Otherwise, return 0.
  */
-int
+bool
 virFileLinkPointsTo(const char *checkLink,
                     const char *checkDest)
 {
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 286401e0f5..92400c18fd 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -176,7 +176,7 @@ int virFileReadBufQuiet(const char *file, char *buf, int len)
 int virFileWriteStr(const char *path, const char *str, mode_t mode)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
 
-int virFileLinkPointsTo(const char *checkLink,
+bool virFileLinkPointsTo(const char *checkLink,
                         const char *checkDest)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 int virFileRelLinkPointsTo(const char *directory,
-- 
2.30.2
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH v2] network: convert virFileLinkPointsTo to return bool
Posted by Martin Kletzander 4 months, 3 weeks ago 
On Thu, Dec 07, 2023 at 12:09:38PM +0300, Anastasia Belova wrote:
>Convert prototype of virFileLinkPointsTo to return bool.
>Remove dead checks in virDomainObjListLoadConfig and
>virNetworkLoadConfig.
>
>Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
>Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
>---
>v2: fix logic according to maintainer's answer
> src/conf/virdomainobjlist.c | 3 +--
> src/conf/virnetworkobj.c    | 3 +--
> src/util/virfile.c          | 2 +-
> src/util/virfile.h          | 2 +-
> 4 files changed, 4 insertions(+), 6 deletions(-)
>
>diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>index 0bd833257d..bb5807d00b 100644
>--- a/src/conf/virdomainobjlist.c
>+++ b/src/conf/virdomainobjlist.c
>@@ -497,8 +497,7 @@ virDomainObjListLoadConfig(virDomainObjList *doms,
>     if ((autostartLink = virDomainConfigFile(autostartDir, name)) == NULL)
>         return NULL;
>
>-    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
>-        return NULL;
>+    autostart = virFileLinkPointsTo(autostartLink, configFile);
>
>     if (!(dom = virDomainObjListAddLocked(doms, &def, xmlopt, 0, &oldDef)))
>         return NULL;
>diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
>index 20ee8eb58a..d5aa121e20 100644
>--- a/src/conf/virnetworkobj.c
>+++ b/src/conf/virnetworkobj.c
>@@ -945,8 +945,7 @@ virNetworkLoadConfig(virNetworkObjList *nets,
>     if ((autostartLink = virNetworkConfigFile(autostartDir, name)) == NULL)
>         return NULL;
>
>-    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
>-        return NULL;
>+    autostart = virFileLinkPointsTo(autostartLink, configFile);
>
>     if (!(def = virNetworkDefParse(NULL, configFile, xmlopt, false)))
>         return NULL;
>diff --git a/src/util/virfile.c b/src/util/virfile.c
>index 007b6cf512..f3108e99cf 100644
>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -1697,7 +1697,7 @@ virFileWriteStr(const char *path, const char *str, mode_t mode)
> /* Return nonzero if checkLink and checkDest
>  * refer to the same file.  Otherwise, return 0.
>  */
>-int
>+bool
> virFileLinkPointsTo(const char *checkLink,
>                     const char *checkDest)
> {
>diff --git a/src/util/virfile.h b/src/util/virfile.h
>index 286401e0f5..92400c18fd 100644
>--- a/src/util/virfile.h
>+++ b/src/util/virfile.h
>@@ -176,7 +176,7 @@ int virFileReadBufQuiet(const char *file, char *buf, int len)
> int virFileWriteStr(const char *path, const char *str, mode_t mode)
>     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
>
>-int virFileLinkPointsTo(const char *checkLink,
>+bool virFileLinkPointsTo(const char *checkLink,
>                         const char *checkDest)

Alignment is off here, but I can fix that before pushing.

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

and thanks for the patch.

>     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> int virFileRelLinkPointsTo(const char *directory,
>-- 
>2.30.2
>_______________________________________________
>Devel mailing list -- devel@lists.libvirt.org
>To unsubscribe send an email to devel-leave@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.