[libvirt] [PATCH 0/3] Multiple fixes related to lookup of gluster volumes
Peter Krempa (3): storage: Don't lie about path used to look up in error message util: file: Don't sanitize URI protocol separator in virFileSanitizePath gluster: Fix "key" attribute for gluster volumes src/storage/storage_backend_gluster.c | 10 ++++++++-- src/storage/storage_driver.c | 12 +++++++++--- src/util/virfile.c | 12 ++++++++++++ 3 files changed, 29 insertions(+), 5 deletions(-) -- 1.8.5.5
In storageVolLookupByPath the provided path is "sanitized" at first. This removes some extra slashes and stuff. When the lookup of the volume fails the original path is used which makes it hard to trace errors in some cases. Improve the error message to print the sanitized path along with the user provided path if they are not equal. --- src/storage/storage_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e0ebdb0..42cb397 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1484,9 +1484,15 @@ storageVolLookupByPath(virConnectPtr conn, virStoragePoolObjUnlock(driver->pools.objs[i]); } - if (!ret) - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching path %s"), path); + if (!ret) { + if (STREQ(path, cleanpath)) { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching path '%s'"), path); + } else { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching path '%s' (%s)"), path, cleanpath); + } + } cleanup: VIR_FREE(cleanpath); -- 1.8.5.5
On 02/24/2014 08:21 AM, Peter Krempa wrote:
In storageVolLookupByPath the provided path is "sanitized" at first. This removes some extra slashes and stuff. When the lookup of the volume fails the original path is used which makes it hard to trace errors in some cases.
Improve the error message to print the sanitized path along with the user provided path if they are not equal. --- src/storage/storage_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
+ _("no storage vol with matching path '%s'"), path); + } else { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching path '%s' (%s)"), path, cleanpath);
Long line; break after the first ',' ACK with that change; makes sense to me to give a bit more information in the error message. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The function removes multiple following slashes from paths. This is okay unless you try to sanitize a URI this way. Skip the protocol definition until "://" and sanitize just the path part. The sanitization function is used in virStorageVolLookupByPath as the first step before passing the path to storage drivers. This breaks lookup of gluster volumes. --- src/util/virfile.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/util/virfile.c b/src/util/virfile.c index 96f078d..6a09609 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2587,6 +2587,8 @@ virFileSanitizePath(const char *path) const char *cur = path; char *cleanpath; int idx = 0; + const char *firstslash; + const char *uriseparator; if (VIR_STRDUP(cleanpath, path) < 0) return NULL; @@ -2596,12 +2598,22 @@ virFileSanitizePath(const char *path) * /// -> / * /../foo -> /../foo * /foo///bar/ -> /foo/bar + * Need to leave in place: + * :// - URIs */ + /* find positions of first colon and first slash */ + firstslash = strchr(path, '/'); + /* Starting with // is valid posix, but ///foo == /foo */ if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') { idx = 2; cur += 2; + } else if ((uriseparator = strstr(path, "://")) && + uriseparator < firstslash) { + + cur = uriseparator + 3; + idx = cur - path; } /* Sanitize path in place */ -- 1.8.5.5
On Mon, Feb 24, 2014 at 04:21:47PM +0100, Peter Krempa wrote:
The function removes multiple following slashes from paths. This is okay unless you try to sanitize a URI this way. Skip the protocol definition until "://" and sanitize just the path part.
The sanitization function is used in virStorageVolLookupByPath as the first step before passing the path to storage drivers. This breaks lookup of gluster volumes. --- src/util/virfile.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
This feels a rather dirty to me - we shouldn't be trying to second guess whether a filename might be a URI. I think we should not be passing URIs to the virFileSanitizePath in the first place. Based on what you say about usage, it sounds like the call to virFileSanitizePath needs to be pushed down into the storage backend drivers themselves. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/24/2014 08:26 AM, Daniel P. Berrange wrote:
On Mon, Feb 24, 2014 at 04:21:47PM +0100, Peter Krempa wrote:
The function removes multiple following slashes from paths. This is okay unless you try to sanitize a URI this way. Skip the protocol definition until "://" and sanitize just the path part.
The sanitization function is used in virStorageVolLookupByPath as the first step before passing the path to storage drivers. This breaks lookup of gluster volumes. --- src/util/virfile.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
This feels a rather dirty to me - we shouldn't be trying to second guess whether a filename might be a URI.
I think we should not be passing URIs to the virFileSanitizePath in the first place. Based on what you say about usage, it sounds like the call to virFileSanitizePath needs to be pushed down into the storage backend drivers themselves.
I agree that this feels dirty; I think it would be better to fix util/virstoragefile to not attempt to sanitize remote names (since right now, all use of URIs are treated as remote names). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
According to our documentation the "key" value has the following meaning: "Providing an identifier for the volume which is globally unique. This cannot be set when creating a volume: it is always generated." The currently used keys for gluster volumes consist of the gluster volume name and file path. This can't be considered unique as a different storage server can serve a volume with the same name. Use the full URI as the key for the volume to avoid ambiguity problems. --- src/storage/storage_backend_gluster.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 202a441..bb13463 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -185,6 +185,7 @@ virStorageBackendGlusterSetMetadata(virStorageBackendGlusterStatePtr state, const char *name) { int ret = -1; + char *path = NULL; char *tmp; VIR_FREE(vol->key); @@ -199,12 +200,12 @@ virStorageBackendGlusterSetMetadata(virStorageBackendGlusterStatePtr state, goto cleanup; } - if (virAsprintf(&vol->key, "%s%s%s", state->volname, state->dir, + if (virAsprintf(&path, "%s%s%s", state->volname, state->dir, vol->name) < 0) goto cleanup; tmp = state->uri->path; - if (virAsprintf(&state->uri->path, "/%s", vol->key) < 0) { + if (virAsprintf(&state->uri->path, "/%s", path) < 0) { state->uri->path = tmp; goto cleanup; } @@ -216,9 +217,14 @@ virStorageBackendGlusterSetMetadata(virStorageBackendGlusterStatePtr state, VIR_FREE(state->uri->path); state->uri->path = tmp; + /* the path is unique enough to serve as a volume key */ + if (VIR_STRDUP(vol->key, vol->target.path) < 0) + goto cleanup; + ret = 0; cleanup: + VIR_FREE(path); return ret; } -- 1.8.5.5
On 02/24/2014 08:21 AM, Peter Krempa wrote:
According to our documentation the "key" value has the following meaning: "Providing an identifier for the volume which is globally unique. This cannot be set when creating a volume: it is always generated." The currently used keys for gluster volumes consist of the gluster volume name and file path. This can't be considered unique as a different storage server can serve a volume with the same name.
Use the full URI as the key for the volume to avoid ambiguity problems.
The full URI cannot be considered unique, either, as both gluster://hosta/volume/file and gluster://hostb/volume/file may resolve to the same file. I think we are better off documenting that a key is unique for some pools, but best effort for others, and not change what we have already been outputting. But if we DO keep this patch, you also need to change the documentation that gives examples of gluster keys. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/24/14 21:59, Eric Blake wrote:
On 02/24/2014 08:21 AM, Peter Krempa wrote:
According to our documentation the "key" value has the following meaning: "Providing an identifier for the volume which is globally unique. This cannot be set when creating a volume: it is always generated." The currently used keys for gluster volumes consist of the gluster volume name and file path. This can't be considered unique as a different storage server can serve a volume with the same name.
Use the full URI as the key for the volume to avoid ambiguity problems.
The full URI cannot be considered unique, either, as both gluster://hosta/volume/file and gluster://hostb/volume/file may resolve to the same file. I think we are better off documenting that a key is
This depends on what we consider as "unique" in this context: The problem you are describing here is that two different keys may map to a single volume. The issue I'm trying to solve is that one key may map to two distinct volumes. As a first step we should thus clarify which way the key should be unique. If we want to make sure the mapping is always just 1:1, we might want to choose the gluster volume UUID as the first part of the key instead of the name.
unique for some pools, but best effort for others, and not change what we have already been outputting. But if we DO keep this patch, you also need to change the documentation that gives examples of gluster keys.
Peter
On 02/25/2014 03:36 AM, Peter Krempa wrote:
The problem you are describing here is that two different keys may map to a single volume. The issue I'm trying to solve is that one key may map to two distinct volumes.
As a first step we should thus clarify which way the key should be unique.
If we want to make sure the mapping is always just 1:1, we might want to choose the gluster volume UUID as the first part of the key instead of the name.
That actually sounds like a reasonable idea - if we are trying to ensure that a key lookup will find exactly one volume across multiple pools, then encoding the pool UUID into the volume key name seems reasonable.
unique for some pools, but best effort for others, and not change what we have already been outputting. But if we DO keep this patch, you also need to change the documentation that gives examples of gluster keys.
This is still true - we'd need to document the choice, as well as mention that early implementations of gluster pools were buggy with regards to key generation. As it would be a bug fix, I think we can still try to get this done in time for 1.2.2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Feb 24, 2014 at 01:59:30PM -0700, Eric Blake wrote:
On 02/24/2014 08:21 AM, Peter Krempa wrote:
According to our documentation the "key" value has the following meaning: "Providing an identifier for the volume which is globally unique. This cannot be set when creating a volume: it is always generated." The currently used keys for gluster volumes consist of the gluster volume name and file path. This can't be considered unique as a different storage server can serve a volume with the same name.
Use the full URI as the key for the volume to avoid ambiguity problems.
The full URI cannot be considered unique, either, as both gluster://hosta/volume/file and gluster://hostb/volume/file may resolve to the same file.
You're talking about a different type of uniqueness here. The important aspect of key uniqueness is that a single 'key' must be unambigous about what volume it is associated with. ie a single 'key' must not be capable of refering to 2 completely different volumes. Your example is about whether each volume has precisely 1 unique key. In the context of the storage pools I believe only the former point is important. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/24/2014 08:21 AM, Peter Krempa wrote:
Peter Krempa (3): storage: Don't lie about path used to look up in error message util: file: Don't sanitize URI protocol separator in virFileSanitizePath gluster: Fix "key" attribute for gluster volumes
I haven't seen this one in my inbox yet but did see it online (don't know where the delay is occurring). My gut reaction is that patch 3/3 is wrong, and that we should instead fix the documentation. We've already released code with 'key' being non-unique and it is likely that clients have already come to rely/deal with that. Changing what we stick in the key now without good justification may be worse than the notion of "fixing it to make things globally unique". Global uniqueness is easy for files in the local file system, but a lot harder for distributed network filesystems. So merely documenting that key may not be unique may be the better approach. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa