12:30 p.m.
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(a)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
2:41 p.m.
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(a)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(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
3:09 a.m.
New subject: [PATCH v2] network: convert virFileLinkPointsTo to return bool
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(a)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
4:32 a.m.
New subject: [PATCH v2] network: convert virFileLinkPointsTo to return bool
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(a)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(a)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(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
353
days inactive
354
days old
3 comments
3 participants
participants (3)
-
Anastasia Belova -
Martin Kletzander -
Peter Krempa