The latter is a glibc extension that's not available on other
operating systems, notably FreeBSD.
So far we have worked around the issue through gnulib, but that
makes it difficult to use mocking in our test suite, so just
drop it in favor of the portable alternative.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/storage/storage_backend_fs.c | 2 +-
src/util/virfile.c | 2 +-
src/util/virpci.c | 2 +-
tests/virstoragetest.c | 10 +++++-----
tests/virtestmock.c | 13 ++++++-------
5 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 9b0fcf92c5..37f02f0712 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -823,7 +823,7 @@ virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src)
virStorageFileBackendFsPrivPtr priv = src->drv->priv;
if (!priv->canonpath) {
- if (!(priv->canonpath = canonicalize_file_name(src->path))) {
+ if (!(priv->canonpath = realpath(src->path, NULL))) {
virReportSystemError(errno, _("can't canonicalize path '%s'"),
src->path);
return NULL;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index e12a584ca1..270906a009 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1597,7 +1597,7 @@ virFileResolveLinkHelper(const char *linkpath,
return VIR_STRDUP_QUIET(*resultpath, linkpath) < 0 ? -1 : 0;
}
- *resultpath = canonicalize_file_name(linkpath);
+ *resultpath = realpath(linkpath, NULL);
return *resultpath == NULL ? -1 : 0;
}
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 23932558e9..1108d2a7c1 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2622,7 +2622,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link)
return NULL;
}
- device_path = canonicalize_file_name(device_link);
+ device_path = realpath(device_link, NULL);
if (device_path == NULL) {
virReportSystemError(errno,
_("Failed to resolve device link '%s'"),
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index b032d8b93f..ff95292698 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -171,7 +171,7 @@ testPrepImages(void)
fprintf(stderr, "unable to create directory %s\n", datadir "/dir");
goto cleanup;
}
- if (!(canondir = canonicalize_file_name(absdir))) {
+ if (!(canondir = realpath(absdir, NULL))) {
virReportOOMError();
goto cleanup;
}
@@ -186,7 +186,7 @@ testPrepImages(void)
fprintf(stderr, "unable to create raw file\n");
goto cleanup;
}
- if (!(canonraw = canonicalize_file_name(absraw))) {
+ if (!(canonraw = realpath(absraw, NULL))) {
virReportOOMError();
goto cleanup;
}
@@ -206,7 +206,7 @@ testPrepImages(void)
"-F", "raw", "-b", "raw", "qcow2", NULL);
if (virCommandRun(cmd, NULL) < 0)
goto skip;
- if (!(canonqcow2 = canonicalize_file_name(absqcow2))) {
+ if (!(canonqcow2 = realpath(absqcow2, NULL))) {
virReportOOMError();
goto cleanup;
}
@@ -220,7 +220,7 @@ testPrepImages(void)
virCommandAddArg(cmd, "wrap");
if (virCommandRun(cmd, NULL) < 0)
goto skip;
- if (!(canonwrap = canonicalize_file_name(abswrap))) {
+ if (!(canonwrap = realpath(abswrap, NULL))) {
virReportOOMError();
goto cleanup;
}
@@ -233,7 +233,7 @@ testPrepImages(void)
virCommandAddArg(cmd, "qed");
if (virCommandRun(cmd, NULL) < 0)
goto skip;
- if (!(canonqed = canonicalize_file_name(absqed))) {
+ if (!(canonqed = realpath(absqed, NULL))) {
virReportOOMError();
goto cleanup;
}
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index 688945d805..9dcff1bd75 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -133,12 +133,11 @@ checkPath(const char *path)
virAsprintfQuiet(&relPath, "./%s", path) < 0)
goto error;
- /* Le sigh. Both canonicalize_file_name() and realpath()
- * expect @path to exist otherwise they return an error. So
- * if we are called over an non-existent file, this could
- * return an error. In that case do our best and hope we will
- * catch possible error. */
- if ((fullPath = canonicalize_file_name(relPath ? relPath : path))) {
+ /* Le sigh. realpath() expects @path to exist, otherwise it will
+ * return an error. So if we are called over an non-existent file,
+ * this could return an error. In that case do our best and hope we
+ * will catch possible errors. */
+ if ((fullPath = realpath(relPath ? relPath : path, NULL))) {
path = fullPath;
} else {
/* Yeah, our worst nightmares just became true. Path does
@@ -148,7 +147,7 @@ checkPath(const char *path)
virFileRemoveLastComponent(crippledPath);
- if ((fullPath = canonicalize_file_name(crippledPath)))
+ if ((fullPath = realpath(crippledPath, NULL)))
path = fullPath;
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Apr 30, 2018 at 06:52:58PM +0200, Andrea Bolognani wrote:
> The latter is a glibc extension that's not available on other
> operating systems, notably FreeBSD.
>
> So far we have worked around the issue through gnulib, but that
> makes it difficult to use mocking in our test suite, so just
> drop it in favor of the portable alternative.
Sigh, unfortunately realpath() has its own portability problems
in that passing NULL as second arg was a glibc invention, which
is why we switched from realpath() to canonicalize_file_name()
in the first place !
So I wonder if this actually works on OS-X, Mingw and FreeBSD,
or whether you didn't see the bug because it is mocked in the
tests ?
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/storage/storage_backend_fs.c | 2 +-
> src/util/virfile.c | 2 +-
> src/util/virpci.c | 2 +-
> tests/virstoragetest.c | 10 +++++-----
> tests/virtestmock.c | 13 ++++++-------
> 5 files changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 9b0fcf92c5..37f02f0712 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -823,7 +823,7 @@ virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src)
> virStorageFileBackendFsPrivPtr priv = src->drv->priv;
>
> if (!priv->canonpath) {
> - if (!(priv->canonpath = canonicalize_file_name(src->path))) {
> + if (!(priv->canonpath = realpath(src->path, NULL))) {
> virReportSystemError(errno, _("can't canonicalize path '%s'"),
> src->path);
> return NULL;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index e12a584ca1..270906a009 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1597,7 +1597,7 @@ virFileResolveLinkHelper(const char *linkpath,
> return VIR_STRDUP_QUIET(*resultpath, linkpath) < 0 ? -1 : 0;
> }
>
> - *resultpath = canonicalize_file_name(linkpath);
> + *resultpath = realpath(linkpath, NULL);
>
> return *resultpath == NULL ? -1 : 0;
> }
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 23932558e9..1108d2a7c1 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -2622,7 +2622,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link)
> return NULL;
> }
>
> - device_path = canonicalize_file_name(device_link);
> + device_path = realpath(device_link, NULL);
> if (device_path == NULL) {
> virReportSystemError(errno,
> _("Failed to resolve device link '%s'"),
> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> index b032d8b93f..ff95292698 100644
> --- a/tests/virstoragetest.c
> +++ b/tests/virstoragetest.c
> @@ -171,7 +171,7 @@ testPrepImages(void)
> fprintf(stderr, "unable to create directory %s\n", datadir "/dir");
> goto cleanup;
> }
> - if (!(canondir = canonicalize_file_name(absdir))) {
> + if (!(canondir = realpath(absdir, NULL))) {
> virReportOOMError();
> goto cleanup;
> }
> @@ -186,7 +186,7 @@ testPrepImages(void)
> fprintf(stderr, "unable to create raw file\n");
> goto cleanup;
> }
> - if (!(canonraw = canonicalize_file_name(absraw))) {
> + if (!(canonraw = realpath(absraw, NULL))) {
> virReportOOMError();
> goto cleanup;
> }
> @@ -206,7 +206,7 @@ testPrepImages(void)
> "-F", "raw", "-b", "raw", "qcow2", NULL);
> if (virCommandRun(cmd, NULL) < 0)
> goto skip;
> - if (!(canonqcow2 = canonicalize_file_name(absqcow2))) {
> + if (!(canonqcow2 = realpath(absqcow2, NULL))) {
> virReportOOMError();
> goto cleanup;
> }
> @@ -220,7 +220,7 @@ testPrepImages(void)
> virCommandAddArg(cmd, "wrap");
> if (virCommandRun(cmd, NULL) < 0)
> goto skip;
> - if (!(canonwrap = canonicalize_file_name(abswrap))) {
> + if (!(canonwrap = realpath(abswrap, NULL))) {
> virReportOOMError();
> goto cleanup;
> }
> @@ -233,7 +233,7 @@ testPrepImages(void)
> virCommandAddArg(cmd, "qed");
> if (virCommandRun(cmd, NULL) < 0)
> goto skip;
> - if (!(canonqed = canonicalize_file_name(absqed))) {
> + if (!(canonqed = realpath(absqed, NULL))) {
> virReportOOMError();
> goto cleanup;
> }
> diff --git a/tests/virtestmock.c b/tests/virtestmock.c
> index 688945d805..9dcff1bd75 100644
> --- a/tests/virtestmock.c
> +++ b/tests/virtestmock.c
> @@ -133,12 +133,11 @@ checkPath(const char *path)
> virAsprintfQuiet(&relPath, "./%s", path) < 0)
> goto error;
>
> - /* Le sigh. Both canonicalize_file_name() and realpath()
> - * expect @path to exist otherwise they return an error. So
> - * if we are called over an non-existent file, this could
> - * return an error. In that case do our best and hope we will
> - * catch possible error. */
> - if ((fullPath = canonicalize_file_name(relPath ? relPath : path))) {
> + /* Le sigh. realpath() expects @path to exist, otherwise it will
> + * return an error. So if we are called over an non-existent file,
> + * this could return an error. In that case do our best and hope we
> + * will catch possible errors. */
> + if ((fullPath = realpath(relPath ? relPath : path, NULL))) {
> path = fullPath;
> } else {
> /* Yeah, our worst nightmares just became true. Path does
> @@ -148,7 +147,7 @@ checkPath(const char *path)
>
> virFileRemoveLastComponent(crippledPath);
>
> - if ((fullPath = canonicalize_file_name(crippledPath)))
> + if ((fullPath = realpath(crippledPath, NULL)))
> path = fullPath;
> }
>
> --
> 2.14.3
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 02, 2018 at 05:32:25PM +0100, Daniel P. Berrangé wrote: > On Mon, Apr 30, 2018 at 06:52:58PM +0200, Andrea Bolognani wrote: > > The latter is a glibc extension that's not available on other > > operating systems, notably FreeBSD. > > > > So far we have worked around the issue through gnulib, but that > > makes it difficult to use mocking in our test suite, so just > > drop it in favor of the portable alternative. > > Sigh, unfortunately realpath() has its own portability problems > in that passing NULL as second arg was a glibc invention, which > is why we switched from realpath() to canonicalize_file_name() > in the first place ! > > So I wonder if this actually works on OS-X, Mingw and FreeBSD, > or whether you didn't see the bug because it is mocked in the > tests ? Hmm, another idea - if the problem is that we can't easily mock canonicalize_file_name() why don't we just provide a trivial wrapper. eg virFileCanonicalize() which does nothing except call canonicalize_file_name(). We can then just mock the virFileCanonicalize() method instead. This would nicely avoid the pain with dealing with versioned symbols for realpath too. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2018-05-02 at 17:32 +0100, Daniel P. Berrangé wrote: > On Mon, Apr 30, 2018 at 06:52:58PM +0200, Andrea Bolognani wrote: > > The latter is a glibc extension that's not available on other > > operating systems, notably FreeBSD. > > > > So far we have worked around the issue through gnulib, but that > > makes it difficult to use mocking in our test suite, so just > > drop it in favor of the portable alternative. > > Sigh, unfortunately realpath() has its own portability problems > in that passing NULL as second arg was a glibc invention, which > is why we switched from realpath() to canonicalize_file_name() > in the first place ! > > So I wonder if this actually works on OS-X, Mingw and FreeBSD, > or whether you didn't see the bug because it is mocked in the > tests ? The FreeBSD[1] and macOS[2] releases we care about ship a sensible implementation of realpath(), which explicitly allows passing NULL as the second parameter. Not sure about MinGW, but doesn't it ship basically a recompiled version of glibc? If so, I would expect it to be perfectly fine too. I should also note that I've successfully run the test suite, with these changes applied, on all CI platforms, so we know there's no Linux distribution we care about shipping an old enough glibc that wouldn't include a reasonable realpath(). As for the mocking hiding the issue, we end up calling the actual realpath() at the end of the day, just on a different file... So I wouldn't expect that to invalidate the results. [1] https://www.freebsd.org/cgi/man.cgi?query=realpath&apropos=0&sektion=3&manpath=FreeBSD+10.4-RELEASE&arch=default&format=html [2] https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/realpath.3.html -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wednesday, 2 May 2018 18:32:25 CEST Daniel P. Berrangé wrote: > On Mon, Apr 30, 2018 at 06:52:58PM +0200, Andrea Bolognani wrote: > > The latter is a glibc extension that's not available on other > > operating systems, notably FreeBSD. > > > > So far we have worked around the issue through gnulib, but that > > makes it difficult to use mocking in our test suite, so just > > drop it in favor of the portable alternative. > > Sigh, unfortunately realpath() has its own portability problems > in that passing NULL as second arg was a glibc invention, which > is why we switched from realpath() to canonicalize_file_name() > in the first place ! It was, but then this behaviour was standardized in POSIX 2007: http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html > So I wonder if this actually works on OS-X, Mingw and FreeBSD, > or whether you didn't see the bug because it is mocked in the > tests ? Most probably because they implement the realpath(.., NULL) semantics, as described by POSIX. -- Pino Toscano-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.