[libvirt] main: fix some compilation issues on non-linux platforms
This patch fixes *some* compilation issues on non-Linux platforms (cygwin). Signed-off-by: Stefan Berger <stefanb@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);
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@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@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;
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@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@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
participants (2)
-
Eric Blake -
Stefan Berger