[PATCH] network: fix check in virNetworkLoadConfig
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
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
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
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
participants (3)
-
Anastasia Belova -
Martin Kletzander -
Peter Krempa