12:19 p.m.
This patch fixes *some* compilation issues on non-Linux platforms (cygwin).
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
diff --git a/src/util/interface.c b/src/util/interface.c
index aec12f5..5d473b7 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -1317,7 +1317,7 @@ ifaceGetPhysicalFunction(const char *ifname, char
**pfname)
}
#else
int
-ifaceIsVirtualFunction(const char *ifname)
+ifaceIsVirtualFunction(const char *ifname ATTRIBUTE_UNUSED)
{
ifaceError(VIR_ERR_INTERNAL_ERROR, "%s",
_("ifaceIsVirtualFunction is not supported on non-linux "
@@ -1326,8 +1326,9 @@ ifaceIsVirtualFunction(const char *ifname)
}
int
-ifaceGetVirtualFunctionIndex(const char *pfname, const char *vfname,
- int *vf_index)
+ifaceGetVirtualFunctionIndex(const char *pfname ATTRIBUTE_UNUSED,
+ const char *vfname ATTRIBUTE_UNUSED,
+ int *vf_index ATTRIBUTE_UNUSED)
{
ifaceError(VIR_ERR_INTERNAL_ERROR, "%s",
_("ifaceGetVirtualFunctionIndex is not supported on
non-linux "
@@ -1336,7 +1337,8 @@ ifaceGetVirtualFunctionIndex(const char *pfname,
const char *vfname,
}
int
-ifaceGetPhysicalFunction(const char *ifname, char **pfname)
+ifaceGetPhysicalFunction(const char *ifname ATTRIBUTE_UNUSED,
+ char **pfname ATTRIBUTE_UNUSED)
{
ifaceError(VIR_ERR_INTERNAL_ERROR, "%s",
_("ifaceGetPhysicalFunction is not supported on non-linux "
diff --git a/src/util/pci.c b/src/util/pci.c
index 7040e55..9873f33 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -1685,6 +1685,8 @@ int pciDeviceIsAssignable(pciDevice *dev,
return 1;
}
+#ifdef __linux__
+
/*
* returns 1 if equal and 0 if not
*/
@@ -1804,7 +1806,6 @@ out:
return ret;
}
-#ifdef __linux__
/*
* Returns Physical function given a virtual function
*/
@@ -2010,8 +2011,8 @@ out:
}
#else
int
-pciGetPhysicalFunction(const char *vf_sysfs_path,
- struct pci_config_address **physical_function)
+pciGetPhysicalFunction(const char *vf_sysfs_path ATTRIBUTE_UNUSED,
+ struct pci_config_address **physical_function
ATTRIBUTE_UNUSED)
{
pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciGetPhysicalFunction
is not "
"supported on non-linux platforms"));
@@ -2019,9 +2020,9 @@ pciGetPhysicalFunction(const char *vf_sysfs_path,
}
int
-pciGetVirtualFunctions(const char *sysfs_path,
- struct pci_config_address ***virtual_functions,
- unsigned int *num_virtual_functions)
+pciGetVirtualFunctions(const char *sysfs_path ATTRIBUTE_UNUSED,
+ struct pci_config_address ***virtual_functions
ATTRIBUTE_UNUSED,
+ unsigned int *num_virtual_functions ATTRIBUTE_UNUSED)
{
pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciGetVirtualFunctions
is not "
"supported on non-linux platforms"));
@@ -2029,7 +2030,7 @@ pciGetVirtualFunctions(const char *sysfs_path,
}
int
-pciDeviceIsVirtualFunction(const char *vf_sysfs_device_link)
+pciDeviceIsVirtualFunction(const char *vf_sysfs_device_link
ATTRIBUTE_UNUSED)
{
pciReportError(VIR_ERR_INTERNAL_ERROR,
_("pciDeviceIsVirtualFunction is "
"not supported on non-linux platforms"));
@@ -2037,9 +2038,9 @@ pciDeviceIsVirtualFunction(const char
*vf_sysfs_device_link)
}
int
-pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
- const char *vf_sysfs_device_link,
- int *vf_index)
+pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link
ATTRIBUTE_UNUSED,
+ const char *vf_sysfs_device_link
ATTRIBUTE_UNUSED,
+ int *vf_index ATTRIBUTE_UNUSED)
{
pciReportError(VIR_ERR_INTERNAL_ERROR,
_("pciGetVirtualFunctionIndex is "
"not supported on non-linux platforms"));
@@ -2048,7 +2049,8 @@ pciGetVirtualFunctionIndex(const char
*pf_sysfs_device_link,
}
int
-pciDeviceNetName(char *device_link_sysfs_path, char **netname)
+pciDeviceNetName(char *device_link_sysfs_path ATTRIBUTE_UNUSED,
+ char **netname ATTRIBUTE_UNUSED)
{
pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciDeviceNetName is not "
"supported on non-linux platforms"));
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 0edf058..1158998 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -44,7 +44,7 @@
int virFileClose(int *fdptr, bool preserve_errno)
{
- int saved_errno;
+ int saved_errno = 0;
int rc = 0;
if (*fdptr >= 0) {
@@ -62,7 +62,7 @@ int virFileClose(int *fdptr, bool preserve_errno)
int virFileFclose(FILE **file, bool preserve_errno)
{
- int saved_errno;
+ int saved_errno = 0;
int rc = 0;
if (*file) {
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 7dd3c51..1927051 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -213,6 +213,8 @@ int virPidFileReadPathIfAlive(const char *path,
#ifdef __linux__
if (virFileLinkPointsTo(procpath, binpath) == 0)
*pid = -1;
+#else
+ (void)binpath;
#endif
VIR_FREE(procpath);
12:29 p.m.
On 08/16/2011 11:19 AM, Stefan Berger wrote:
s/main/maint/ in the subject line.
This patch fixes *some* compilation issues on non-Linux platforms
(cygwin).
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
int
-pciDeviceNetName(char *device_link_sysfs_path, char **netname)
+pciDeviceNetName(char *device_link_sysfs_path ATTRIBUTE_UNUSED,
+ char **netname ATTRIBUTE_UNUSED)
This and above are fine.
{
pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciDeviceNetName is not "
"supported on non-linux platforms"));
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 0edf058..1158998 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -44,7 +44,7 @@
int virFileClose(int *fdptr, bool preserve_errno)
{
- int saved_errno;
+ int saved_errno = 0;
int rc = 0;
This is a different kind of warning. It is due to a compiler
false-positive (I'm guessing that you are seeing a warning about
uninitialized use of saved_errno, even though use of that variable is
properly gated so that it is initialized on all paths where it is later
used). But still okay, although I might have split it into a different
patch, and listed the compiler warning that it silences as part of the
commit message.
+++ b/src/util/virpidfile.c
@@ -213,6 +213,8 @@ int virPidFileReadPathIfAlive(const char *path,
#ifdef __linux__
if (virFileLinkPointsTo(procpath, binpath) == 0)
*pid = -1;
+#else
+ (void)binpath;
#endif
Here, I'd rather mark binpath as ATTRIBUTE_UNUSED than add the #else
preprocessor conditional. That is, ATTRIBUTE_UNUSED means only that the
variable might be unused, not that it can't be used (so used on linux,
unused otherwise qualifies).
ACK with that nit fixed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:35 p.m.
New subject: [libvirt] main: fix some compilation issues on non-linux platforms
On 08/16/2011 11:29 AM, Eric Blake wrote:
> +++ b/src/util/virpidfile.c
> @@ -213,6 +213,8 @@ int virPidFileReadPathIfAlive(const char *path,
> #ifdef __linux__
> if (virFileLinkPointsTo(procpath, binpath) == 0)
> *pid = -1;
> +#else
> + (void)binpath;
> #endif
Here, I'd rather mark binpath as ATTRIBUTE_UNUSED than add the #else
preprocessor conditional. That is, ATTRIBUTE_UNUSED means only that the
variable might be unused, not that it can't be used (so used on linux,
unused otherwise qualifies).
Food for a separate patch: I see no reason why virFileLinkPointsTo won't
work on cygwin. That is, the fact that we used #ifdef __linux__ is
wrong, since cygwin also has a working /proc/pid/ file tree to determine
if an executable is still valid; we should instead be doing #if
PROCFS_AVAIL, along with some sort of configuration check that accepts
both linux and cygwin procfs (or at least whatever aspect of procfs that
we are relying on in this function).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:07 p.m.
On 08/16/2011 01:35 PM, Eric Blake wrote:
On 08/16/2011 11:29 AM, Eric Blake wrote:
>> +++ b/src/util/virpidfile.c
>> @@ -213,6 +213,8 @@ int virPidFileReadPathIfAlive(const char *path,
>> #ifdef __linux__
>> if (virFileLinkPointsTo(procpath, binpath) == 0)
>> *pid = -1;
>> +#else
>> + (void)binpath;
>> #endif
>
> Here, I'd rather mark binpath as ATTRIBUTE_UNUSED than add the #else
> preprocessor conditional. That is, ATTRIBUTE_UNUSED means only that the
> variable might be unused, not that it can't be used (so used on linux,
> unused otherwise qualifies).
Food for a separate patch: I see no reason why virFileLinkPointsTo
won't work on cygwin. That is, the fact that we used #ifdef __linux__
is wrong, since cygwin also has a working /proc/pid/ file tree to
determine if an executable is still valid; we should instead be doing
#if PROCFS_AVAIL, along with some sort of configuration check that
accepts both linux and cygwin procfs (or at least whatever aspect of
procfs that we are relying on in this function).
I split this off and pushed the rest.
If procfs's are different, then maybe we should use #if
PROCFS_PID_EXE_LINK_AVAIL here.
What is needed in this case seems to be that /proc/<pid>/exe is a
symbolic link to the executable. This works in Linux and cygwin. Would
testing via
stat /proc/<pid>/exe | sed -n 's/.*\(symbolic link\).*/\1/p'
be a valid test for this that works on all targeted platforms?
Stefan
1:14 p.m.
On 08/16/2011 12:07 PM, Stefan Berger wrote:
I split this off and pushed the rest.
If procfs's are different, then maybe we should use #if
PROCFS_PID_EXE_LINK_AVAIL here.
Seems like a reasonable name.
What is needed in this case seems to be that /proc/<pid>/exe is
a
symbolic link to the executable. This works in Linux and cygwin. Would
testing via
stat /proc/<pid>/exe | sed -n 's/.*\(symbolic link\).*/\1/p'
be a valid test for this that works on all targeted platforms?
stat(1) is not universal, and while it is on both Linux and Cygwin, it
might not be present on some other platform that has a procfs that does
what we want. It may be sufficient to just use 'test -h /proc/pid/exe'
to see if there is a symlink.
Also, you'd need to wrap the test in a cache variable (to allow
overrides if we guessed wrong), as well as to skip the test (and assume
the worst or at least make some default guesses based on known $host_os)
when cross-compiling, since the existence of procfs on the host machine
says nothing about it being on the target machine.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:34 p.m.
On 08/16/2011 02:14 PM, Eric Blake wrote:
On 08/16/2011 12:07 PM, Stefan Berger wrote:
> I split this off and pushed the rest.
> If procfs's are different, then maybe we should use #if
> PROCFS_PID_EXE_LINK_AVAIL here.
Seems like a reasonable name.
> What is needed in this case seems to be that /proc/<pid>/exe is a
> symbolic link to the executable. This works in Linux and cygwin. Would
> testing via
>
> stat /proc/<pid>/exe | sed -n 's/.*\(symbolic link\).*/\1/p'
>
> be a valid test for this that works on all targeted platforms?
stat(1) is not universal, and while it is on both Linux and Cygwin, it
might not be present on some other platform that has a procfs that
does what we want. It may be sufficient to just use 'test -h
/proc/pid/exe' to see if there is a symlink.
Also, you'd need to wrap the test in a cache variable (to allow
overrides if we guessed wrong), as well as to skip the test (and
assume the worst or at least make some default guesses based on known
$host_os) when cross-compiling, since the existence of procfs on the
host machine says nothing about it being on the target machine.
Following the latter couldn't it just be handled during runtime altogether?
Stefan
2:01 p.m.
On 08/16/2011 02:34 PM, Stefan Berger wrote:
On 08/16/2011 02:14 PM, Eric Blake wrote:
> On 08/16/2011 12:07 PM, Stefan Berger wrote:
>> I split this off and pushed the rest.
>> If procfs's are different, then maybe we should use #if
>> PROCFS_PID_EXE_LINK_AVAIL here.
>
> Seems like a reasonable name.
>
>> What is needed in this case seems to be that /proc/<pid>/exe is a
>> symbolic link to the executable. This works in Linux and cygwin. Would
>> testing via
>>
>> stat /proc/<pid>/exe | sed -n 's/.*\(symbolic link\).*/\1/p'
>>
>> be a valid test for this that works on all targeted platforms?
>
> stat(1) is not universal, and while it is on both Linux and Cygwin,
> it might not be present on some other platform that has a procfs that
> does what we want. It may be sufficient to just use 'test -h
> /proc/pid/exe' to see if there is a symlink.
>
> Also, you'd need to wrap the test in a cache variable (to allow
> overrides if we guessed wrong), as well as to skip the test (and
> assume the worst or at least make some default guesses based on known
> $host_os) when cross-compiling, since the existence of procfs on the
> host machine says nothing about it being on the target machine.
>
Following the latter couldn't it just be handled during runtime
altogether?
Stefan
Along those lines I propose this patch below:
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
---
src/libvirt_private.syms | 1 +
src/util/util.c | 17 +++++++++++++++++
src/util/util.h | 2 ++
src/util/virpidfile.c | 7 ++++---
4 files changed, 24 insertions(+), 3 deletions(-)
Index: libvirt-acl/src/util/virpidfile.c
===================================================================
--- libvirt-acl.orig/src/util/virpidfile.c
+++ libvirt-acl/src/util/virpidfile.c
@@ -210,10 +210,11 @@ int virPidFileReadPathIfAlive(const char
*pid = -1;
return 0;
}
-#ifdef __linux__
- if (virFileLinkPointsTo(procpath, binpath) == 0)
+
+ if (virFileIsLink(procpath) &&
+ virFileLinkPointsTo(procpath, binpath) == 0)
*pid = -1;
-#endif
+
VIR_FREE(procpath);
return 0;
Index: libvirt-acl/src/util/util.c
===================================================================
--- libvirt-acl.orig/src/util/util.c
+++ libvirt-acl/src/util/util.c
@@ -570,6 +570,23 @@ int virFileResolveLink(const char *linkp
return *resultpath == NULL ? -1 : 0;
}
+
+/*
+ * Check whether the given file is a link.
+ * Returns 1 in case of the file being a link, 0 in case it is not
+ * a link and the negative errno in all other cases.
+ */
+int virFileIsLink(const char *linkpath)
+{
+ struct stat st;
+
+ if (lstat(linkpath, &st) < 0)
+ return -errno;
+
+ return S_ISLNK(st.st_mode);
+}
+
+
/*
* Finds a requested executable file in the PATH env. e.g.:
* "kvm-img" will return "/usr/bin/kvm-img"
Index: libvirt-acl/src/util/util.h
===================================================================
--- libvirt-acl.orig/src/util/util.h
+++ libvirt-acl/src/util/util.h
@@ -78,6 +78,8 @@ int virFileLinkPointsTo(const char *chec
int virFileResolveLink(const char *linkpath,
char **resultpath) ATTRIBUTE_RETURN_CHECK;
+int virFileIsLink(const char *linkpath) ATTRIBUTE_RETURN_CHECK;
+
char *virFindFileInPath(const char *file);
bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1);
Index: libvirt-acl/src/libvirt_private.syms
===================================================================
--- libvirt-acl.orig/src/libvirt_private.syms
+++ libvirt-acl/src/libvirt_private.syms
@@ -1047,6 +1047,7 @@ virFileExists;
virFileFindMountPoint;
virFileHasSuffix;
virFileIsExecutable;
+virFileIsLink;
virFileLinkPointsTo;
virFileLock;
virFileMakePath;
2:25 p.m.
On 08/16/2011 01:01 PM, Stefan Berger wrote:
> Following the latter couldn't it just be handled during
runtime
> altogether?
That actually sounds better.
>
> Stefan
>
Along those lines I propose this patch below:
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
+int virFileIsLink(const char *linkpath)
+{
+ struct stat st;
+
+ if (lstat(linkpath, &st) < 0)
+ return -errno;
+
+ return S_ISLNK(st.st_mode);
S_ISLNK is only guaranteed to return non-zero for links, not 1. You
need to sanitize it:
return S_ISLNK(st.st_mode) != 0;
or
return !!S_ISLNK(st.st_mode);
+++ libvirt-acl/src/util/util.h
@@ -78,6 +78,8 @@ int virFileLinkPointsTo(const char *chec
int virFileResolveLink(const char *linkpath,
char **resultpath) ATTRIBUTE_RETURN_CHECK;
+int virFileIsLink(const char *linkpath) ATTRIBUTE_RETURN_CHECK;
Also add ATTRIBUTE_NONNULL(1), since lstat(NULL) is invalid.
ACK with those two fixes.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:42 p.m.
On 08/16/2011 03:25 PM, Eric Blake wrote:
On 08/16/2011 01:01 PM, Stefan Berger wrote:
>> Following the latter couldn't it just be handled during runtime
>> altogether?
That actually sounds better.
>>
>> Stefan
>>
> Along those lines I propose this patch below:
>
> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
>
> +int virFileIsLink(const char *linkpath)
> +{
> + struct stat st;
> +
> + if (lstat(linkpath, &st) < 0)
> + return -errno;
> +
> + return S_ISLNK(st.st_mode);
S_ISLNK is only guaranteed to return non-zero for links, not 1. You
need to sanitize it:
return S_ISLNK(st.st_mode) != 0;
or
return !!S_ISLNK(st.st_mode);
> +++ libvirt-acl/src/util/util.h
> @@ -78,6 +78,8 @@ int virFileLinkPointsTo(const char *chec
> int virFileResolveLink(const char *linkpath,
> char **resultpath) ATTRIBUTE_RETURN_CHECK;
>
> +int virFileIsLink(const char *linkpath) ATTRIBUTE_RETURN_CHECK;
Also add ATTRIBUTE_NONNULL(1), since lstat(NULL) is invalid.
ACK with those two fixes.
Pushed with those two fixes.
Stefan
4848
days inactive
4848
days old
8 comments
2 participants
participants (2)
-
Eric Blake -
Stefan Berger