9:21 a.m.
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
9:21 a.m.
New subject: [libvirt] [PATCH 1/3] storage: Don't lie about path used to look up in error message
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
11:58 a.m.
New subject: [libvirt] [PATCH 1/3] storage: Don't lie about path used to look up in error message
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
9:21 a.m.
New subject: [libvirt] [PATCH 2/3] util: file: Don't sanitize URI protocol separator in virFileSanitizePath
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
9:26 a.m.
New subject: [libvirt] [PATCH 2/3] util: file: Don't sanitize URI protocol separator in virFileSanitizePath
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 :|
noon
New subject: [libvirt] [PATCH 2/3] util: file: Don't sanitize URI protocol separator in virFileSanitizePath
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
9:21 a.m.
New subject: [libvirt] [PATCH 3/3] gluster: Fix "key" attribute for gluster volumes
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
2:59 p.m.
New subject: [libvirt] [PATCH 3/3] gluster: Fix "key" attribute for gluster volumes
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
4:36 a.m.
New subject: [libvirt] [PATCH 3/3] gluster: Fix "key" attribute for gluster volumes
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
7:50 a.m.
New subject: [libvirt] [PATCH 3/3] gluster: Fix "key" attribute for gluster volumes
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
5:20 a.m.
New subject: [libvirt] [PATCH 3/3] gluster: Fix "key" attribute for gluster volumes
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 :|
12:06 p.m.
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
3983
days inactive
3984
days old
11 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa