[libvirt] [PATCH 0/3] Couple of qemu NS fixes
Yet again, some corner cases, nothing critical. But it is certainly nice to fix them regardless. Michal Privoznik (3): qemuDomainBuildNamespace: Clean up temp files qemuDomainGetPreservedMounts: Prune nested mount points qemuDomainGetPreservedMounts: Fix suffixes for corner cases src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) -- 2.13.0
https://bugzilla.redhat.com/show_bug.cgi?id=1431112 After 290a00e41d we know how to deal with file mount points. However, when cleaning up the temporary location for preserved mount points we are still calling rmdir(). This won't fly for files. We need to call unlink(). Now, since we don't really care if the cleanup succeeded or not (it's the best effort anyway), we can call both rmdir() and unlink() without need for differentiation between files and directories. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 36fa450e8..23b92606e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8311,8 +8311,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, ret = 0; cleanup: - for (i = 0; i < ndevMountsPath; i++) + for (i = 0; i < ndevMountsPath; i++) { + /* The path can be either a regular file or a dir. */ rmdir(devMountsSavePath[i]); + unlink(devMountsSavePath[i]); + } virStringListFreeCount(devMountsPath, ndevMountsPath); virStringListFreeCount(devMountsSavePath, ndevMountsPath); return ret; -- 2.13.0
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
After 290a00e41d we know how to deal with file mount points. However, when cleaning up the temporary location for preserved mount points we are still calling rmdir(). This won't fly for files. We need to call unlink(). Now, since we don't really care if the cleanup succeeded or not (it's the best effort anyway), we can call both rmdir() and unlink() without need for differentiation between files and directories.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
But why call both? My recollection on this is that unlink() was preferred over rmdir() unless you cared about the target not existing. It's also 'kinder' if some existing reference was left on the file. I would prefer just the unlink for my: Reviewed-by: John Ferlan <jferlan@redhat.com> but you can convince me for having both as well... John Feels like an eblake question, but he's away for a couple weeks, sigh.
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 36fa450e8..23b92606e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8311,8 +8311,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
ret = 0; cleanup: - for (i = 0; i < ndevMountsPath; i++) + for (i = 0; i < ndevMountsPath; i++) { + /* The path can be either a regular file or a dir. */ rmdir(devMountsSavePath[i]); + unlink(devMountsSavePath[i]); + } virStringListFreeCount(devMountsPath, ndevMountsPath); virStringListFreeCount(devMountsSavePath, ndevMountsPath); return ret;
On 06/14/2017 09:50 PM, John Ferlan wrote:
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
After 290a00e41d we know how to deal with file mount points. However, when cleaning up the temporary location for preserved mount points we are still calling rmdir(). This won't fly for files. We need to call unlink(). Now, since we don't really care if the cleanup succeeded or not (it's the best effort anyway), we can call both rmdir() and unlink() without need for differentiation between files and directories.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
But why call both?
I don't think you can call unlink() over a directory, can you? And sure, I could call stat() just to find out if it's a dir or a file and call just one of the pair. Or I can call both and ignore any errors. The result is the same, isn't it? Michal
On 06/15/2017 04:53 AM, Michal Privoznik wrote:
On 06/14/2017 09:50 PM, John Ferlan wrote:
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
After 290a00e41d we know how to deal with file mount points. However, when cleaning up the temporary location for preserved mount points we are still calling rmdir(). This won't fly for files. We need to call unlink(). Now, since we don't really care if the cleanup succeeded or not (it's the best effort anyway), we can call both rmdir() and unlink() without need for differentiation between files and directories.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
But why call both?
I don't think you can call unlink() over a directory, can you? And sure, I could call stat() just to find out if it's a dir or a file and call just one of the pair. Or I can call both and ignore any errors. The result is the same, isn't it?
Michal
From the unlink(3p) man page:
"The path argument shall not name a directory unless the process has appropriate privileges and the implementation supports using unlink() on directories." Then a google search on using unlink vs. rmdir uncovers more refs. I suppose one could also do the "if file, then unlink else rmdir. Just seems "odd" to see both and leaves one wondering why. John
On 06/15/2017 02:03 PM, John Ferlan wrote:
On 06/15/2017 04:53 AM, Michal Privoznik wrote:
On 06/14/2017 09:50 PM, John Ferlan wrote:
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
After 290a00e41d we know how to deal with file mount points. However, when cleaning up the temporary location for preserved mount points we are still calling rmdir(). This won't fly for files. We need to call unlink(). Now, since we don't really care if the cleanup succeeded or not (it's the best effort anyway), we can call both rmdir() and unlink() without need for differentiation between files and directories.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
But why call both?
I don't think you can call unlink() over a directory, can you? And sure, I could call stat() just to find out if it's a dir or a file and call just one of the pair. Or I can call both and ignore any errors. The result is the same, isn't it?
Michal
From the unlink(3p) man page:
"The path argument shall not name a directory unless the process has appropriate privileges and the implementation supports using unlink() on directories."
That's weird. I don't see that in my man page. What I see though is the following errno code: EISDIR pathname refers to a directory. (This is the non-POSIX value returned by Linux since 2.1.132.) The "non-POSIX" bothers me there. But I agree that whole namespaces are Linux specific, so it doesn't bother me there that much.
Then a google search on using unlink vs. rmdir uncovers more refs. I suppose one could also do the "if file, then unlink else rmdir.
Well, to determine if a path is a file or a dir I'd need to call stat() which can fail. I'm not a big fan of overcomplicating simple code.
Just seems "odd" to see both and leaves one wondering why.
Well, there's a comment that says why. Michal
On 06/15/2017 09:44 AM, Michal Privoznik wrote:
On 06/15/2017 02:03 PM, John Ferlan wrote:
On 06/15/2017 04:53 AM, Michal Privoznik wrote:
On 06/14/2017 09:50 PM, John Ferlan wrote:
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
After 290a00e41d we know how to deal with file mount points. However, when cleaning up the temporary location for preserved mount points we are still calling rmdir(). This won't fly for files. We need to call unlink(). Now, since we don't really care if the cleanup succeeded or not (it's the best effort anyway), we can call both rmdir() and unlink() without need for differentiation between files and directories.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
But why call both?
I don't think you can call unlink() over a directory, can you? And sure, I could call stat() just to find out if it's a dir or a file and call just one of the pair. Or I can call both and ignore any errors. The result is the same, isn't it?
Michal
From the unlink(3p) man page:
"The path argument shall not name a directory unless the process has appropriate privileges and the implementation supports using unlink() on directories."
That's weird. I don't see that in my man page. What I see though is the following errno code:
EISDIR pathname refers to a directory. (This is the non-POSIX value returned by Linux since 2.1.132.)
The "non-POSIX" bothers me there. But I agree that whole namespaces are Linux specific, so it doesn't bother me there that much.
I got that from my Fedora install... But I know I've been down this path before using other U*x OS's - those details have long since left my short term memory. Like I said, eblake would be of help here, but he's away for another week or so...
Then a google search on using unlink vs. rmdir uncovers more refs. I suppose one could also do the "if file, then unlink else rmdir.
Well, to determine if a path is a file or a dir I'd need to call stat() which can fail. I'm not a big fan of overcomplicating simple code.
Just seems "odd" to see both and leaves one wondering why.
Well, there's a comment that says why.
Michal
I recall going down this path before when I updated virFileRemove when dealing with NFS and the craziness that is a root-squash environment. I would think the following would work just fine in this case (as cut-n-paste'd from virFileRemove): if (virFileIsDir(path)) return rmdir(path); else return unlink(path); You're ignoring errors anyway so whether stat() fails [in virFileIsDir] or whether unlink() fails because it's being run on a directory in the event that virFileIsDir() fails won't matter. But at least you wouldn't always cause some failure. The way you've changed things you'll always get an error that's ignored. If rmdir() succeeded, then unlink() would fail (on directory). If rmdir() fails (on file), then unlink() could/should/would succeed. John
https://bugzilla.redhat.com/show_bug.cgi?id=1431112 There can be nested mount points. For instance /dev/shm/blah can be a mount point and /dev/shm too. It doesn't make much sense to return the former path because callers preserve the latter (and with that the former too). Therefore prune nested mount points. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 23b92606e..accf05a6f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7533,7 +7533,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, size_t *ndevPath) { char **paths = NULL, **mounts = NULL; - size_t i, nmounts; + size_t i, j, nmounts; if (virFileGetMountSubtree(PROC_MOUNTS, "/dev", &mounts, &nmounts) < 0) @@ -7545,6 +7545,27 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, return 0; } + /* There can be nested mount points. For instance + * /dev/shm/blah can be a mount point and /dev/shm too. It + * doesn't make much sense to return the former path because + * caller preserves the latter (and with that the former + * too). Therefore prune nested mount points. + * NB mounts[0] is "/dev". Should we start the outer loop + * from the beginning of the array all we'd be left with is + * just the first element. Think about it. + */ + for (i = 1; i < nmounts; i++) { + for (j = i + 1; j < nmounts;) { + if (STRPREFIX(mounts[j], mounts[i])) { + VIR_DEBUG("Dropping path %s because of %s", mounts[j], mounts[i]); + VIR_DELETE_ELEMENT(mounts, j, nmounts); + } else { + j++; + } + } + } + + if (VIR_ALLOC_N(paths, nmounts) < 0) goto error; -- 2.13.0
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
There can be nested mount points. For instance /dev/shm/blah can be a mount point and /dev/shm too. It doesn't make much sense to return the former path because callers preserve the latter (and with that the former too). Therefore prune nested mount points.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 23b92606e..accf05a6f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7533,7 +7533,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, size_t *ndevPath) { char **paths = NULL, **mounts = NULL; - size_t i, nmounts; + size_t i, j, nmounts;
if (virFileGetMountSubtree(PROC_MOUNTS, "/dev", &mounts, &nmounts) < 0) @@ -7545,6 +7545,27 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, return 0; }
+ /* There can be nested mount points. For instance + * /dev/shm/blah can be a mount point and /dev/shm too. It + * doesn't make much sense to return the former path because + * caller preserves the latter (and with that the former + * too). Therefore prune nested mount points. + * NB mounts[0] is "/dev". Should we start the outer loop + * from the beginning of the array all we'd be left with is + * just the first element. Think about it. + */ + for (i = 1; i < nmounts; i++) { + for (j = i + 1; j < nmounts;) { + if (STRPREFIX(mounts[j], mounts[i])) { + VIR_DEBUG("Dropping path %s because of %s", mounts[j], mounts[i]); + VIR_DELETE_ELEMENT(mounts, j, nmounts); + } else { + j++; + }
Ewww I prefer a : j = i + 1; while (j < nmounts) { if () ... else j++; } IDC either way, it's the empty last for condition that causes my eyes to roll! Besides it just is ripe for someone coming along and moving that j++ up into the for as a fix which we both know would be a bad idea. Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ } + } + + if (VIR_ALLOC_N(paths, nmounts) < 0) goto error;
On 06/14/2017 09:52 PM, John Ferlan wrote:
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
There can be nested mount points. For instance /dev/shm/blah can be a mount point and /dev/shm too. It doesn't make much sense to return the former path because callers preserve the latter (and with that the former too). Therefore prune nested mount points.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 23b92606e..accf05a6f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7533,7 +7533,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, size_t *ndevPath) { char **paths = NULL, **mounts = NULL; - size_t i, nmounts; + size_t i, j, nmounts;
if (virFileGetMountSubtree(PROC_MOUNTS, "/dev", &mounts, &nmounts) < 0) @@ -7545,6 +7545,27 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, return 0; }
+ /* There can be nested mount points. For instance + * /dev/shm/blah can be a mount point and /dev/shm too. It + * doesn't make much sense to return the former path because + * caller preserves the latter (and with that the former + * too). Therefore prune nested mount points. + * NB mounts[0] is "/dev". Should we start the outer loop + * from the beginning of the array all we'd be left with is + * just the first element. Think about it. + */ + for (i = 1; i < nmounts; i++) { + for (j = i + 1; j < nmounts;) { + if (STRPREFIX(mounts[j], mounts[i])) { + VIR_DEBUG("Dropping path %s because of %s", mounts[j], mounts[i]); + VIR_DELETE_ELEMENT(mounts, j, nmounts); + } else { + j++; + }
Ewww
I prefer a :
j = i + 1; while (j < nmounts) { if () ... else j++; }
Okay. I prefer the other approach (what I have here), but I don't care that much. Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1431112 Imagine a FS mounted on /dev/blah/blah2. Our process of creating suffix for temporary location where all the mounted filesystems are moved is very simplistic. We want: /var/run/libvirt/qemu/$domName.$suffix\ were $suffix is just the mount point path stripped of the "/dev/" preffix. For instance: /var/run/libvirt/qemu/fedora.mqueue for /dev/mqueue /var/run/libvirt/qemu/fedora.pts for /dev/pts and so on. Now if we plug /dev/blah/blah2 into the example we see some misbehaviour: /var/run/libvirt/qemu/fedora.blah/blah2 Well, misbehaviour if /dev/blah/blah2 is a file, because in that case we call virFileTouch() instead of virFileMakePath(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index accf05a6f..547c9fbfb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7570,7 +7570,9 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, goto error; for (i = 0; i < nmounts; i++) { + char *tmp; const char *suffix = mounts[i] + strlen(DEVPREFIX); + size_t off; if (STREQ(mounts[i], "/dev")) suffix = "dev"; @@ -7578,6 +7580,20 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, if (virAsprintf(&paths[i], "%s/%s.%s", cfg->stateDir, vm->def->name, suffix) < 0) goto error; + + /* Now consider that mounts[i] is "/dev/blah/blah2". + * @suffix then points to "blah/blah2". However, caller + * expects all the @paths to be the same depth. The + * caller doesn't always do `mkdir -p` but sometimes bare + * `touch`. Therefore fix all the suffixes. */ + off = strlen(paths[i]) - strlen(suffix); + + tmp = paths[i] + off; + while (*tmp) { + if (*tmp == '/') + *tmp = '.'; + tmp++; + } } if (devPath) -- 2.13.0
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
Imagine a FS mounted on /dev/blah/blah2. Our process of creating suffix for temporary location where all the mounted filesystems are moved is very simplistic. We want:
/var/run/libvirt/qemu/$domName.$suffix\
were $suffix is just the mount point path stripped of the "/dev/" preffix. For instance:
s/preffix/prefix
/var/run/libvirt/qemu/fedora.mqueue for /dev/mqueue /var/run/libvirt/qemu/fedora.pts for /dev/pts
and so on. Now if we plug /dev/blah/blah2 into the example we see some misbehaviour:
/var/run/libvirt/qemu/fedora.blah/blah2
Well, misbehaviour if /dev/blah/blah2 is a file, because in that case we call virFileTouch() instead of virFileMakePath().
You didn't finish my bedtime story! Am I to assume that instead of : /var/run/libvirt/qemu/fedora.blah/blah2 we would get /var/run/libvirt/qemu/fedora.blah.blah2 taking things one step further... would /dev/blah/blah2/blah3 be /var/run/libvirt/qemu/fedora.blah.blah2.blah3 That's what I see coded at least... Or should the path be: /var/run/libvirt/qemu/fedora.blah/blah2.blah3 It would seem you'd want to get to the end, reverse search on '/' then if that spot is greater than @off, then convert it to a '.', but what do I know. I keep to the simple life and don't use namespaces. John
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index accf05a6f..547c9fbfb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7570,7 +7570,9 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, goto error;
for (i = 0; i < nmounts; i++) { + char *tmp; const char *suffix = mounts[i] + strlen(DEVPREFIX); + size_t off;
if (STREQ(mounts[i], "/dev")) suffix = "dev"; @@ -7578,6 +7580,20 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, if (virAsprintf(&paths[i], "%s/%s.%s", cfg->stateDir, vm->def->name, suffix) < 0) goto error; + + /* Now consider that mounts[i] is "/dev/blah/blah2". + * @suffix then points to "blah/blah2". However, caller + * expects all the @paths to be the same depth. The + * caller doesn't always do `mkdir -p` but sometimes bare + * `touch`. Therefore fix all the suffixes. */ + off = strlen(paths[i]) - strlen(suffix); + + tmp = paths[i] + off; + while (*tmp) { + if (*tmp == '/') + *tmp = '.'; + tmp++; + } }
if (devPath)
On 06/14/2017 09:58 PM, John Ferlan wrote:
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
Imagine a FS mounted on /dev/blah/blah2. Our process of creating suffix for temporary location where all the mounted filesystems are moved is very simplistic. We want:
/var/run/libvirt/qemu/$domName.$suffix\
were $suffix is just the mount point path stripped of the "/dev/" preffix. For instance:
s/preffix/prefix
/var/run/libvirt/qemu/fedora.mqueue for /dev/mqueue /var/run/libvirt/qemu/fedora.pts for /dev/pts
and so on. Now if we plug /dev/blah/blah2 into the example we see some misbehaviour:
/var/run/libvirt/qemu/fedora.blah/blah2
Well, misbehaviour if /dev/blah/blah2 is a file, because in that case we call virFileTouch() instead of virFileMakePath().
You didn't finish my bedtime story!
Am I to assume that instead of :
/var/run/libvirt/qemu/fedora.blah/blah2
we would get
/var/run/libvirt/qemu/fedora.blah.blah2
Yes.
taking things one step further...
would /dev/blah/blah2/blah3
be
/var/run/libvirt/qemu/fedora.blah.blah2.blah3
Yes.
That's what I see coded at least... Or should the path be:
/var/run/libvirt/qemu/fedora.blah/blah2.blah3
Nope. The former one.
It would seem you'd want to get to the end, reverse search on '/' then if that spot is greater than @off, then convert it to a '.',
So basically, this is my approach just reversed. What'd be the benefits? I find my algorithm small and easy to understand.
but what do I know. I keep to the simple life and don't use namespaces.
Well, until a7cc039dc I didn't know that you can bind mount files. What a strange thing to learn. What I want to say - you can learn some new stuff when using namespaces ;-) Michal
On 06/15/2017 04:53 AM, Michal Privoznik wrote:
On 06/14/2017 09:58 PM, John Ferlan wrote:
On 06/12/2017 11:57 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
Imagine a FS mounted on /dev/blah/blah2. Our process of creating suffix for temporary location where all the mounted filesystems are moved is very simplistic. We want:
/var/run/libvirt/qemu/$domName.$suffix\
were $suffix is just the mount point path stripped of the "/dev/" preffix. For instance:
s/preffix/prefix
/var/run/libvirt/qemu/fedora.mqueue for /dev/mqueue /var/run/libvirt/qemu/fedora.pts for /dev/pts
and so on. Now if we plug /dev/blah/blah2 into the example we see some misbehaviour:
/var/run/libvirt/qemu/fedora.blah/blah2
Well, misbehaviour if /dev/blah/blah2 is a file, because in that case we call virFileTouch() instead of virFileMakePath().
You didn't finish my bedtime story!
Am I to assume that instead of :
/var/run/libvirt/qemu/fedora.blah/blah2
we would get
/var/run/libvirt/qemu/fedora.blah.blah2
Yes.
taking things one step further...
would /dev/blah/blah2/blah3
be
/var/run/libvirt/qemu/fedora.blah.blah2.blah3
Yes.
That's what I see coded at least... Or should the path be:
/var/run/libvirt/qemu/fedora.blah/blah2.blah3
Nope. The former one.
It would seem you'd want to get to the end, reverse search on '/' then if that spot is greater than @off, then convert it to a '.',
So basically, this is my approach just reversed. What'd be the benefits? I find my algorithm small and easy to understand.
but what do I know. I keep to the simple life and don't use namespaces.
Well, until a7cc039dc I didn't know that you can bind mount files. What a strange thing to learn. What I want to say - you can learn some new stuff when using namespaces ;-)
Michal
OK fair enough - be sure to finish the bed time story that this patch converts @suffix directory 'layers' into the flat namespace using '.' instead of '/'. The whole comment for 'mounts[i] is ...' could be simplified to indicate that we're turning the complete @suffix from a possible multi-directory level into a single flat file reference. Reviewed-by: John Ferlan <jferlan@redhat.com> Just so you know IDC if you keep the for (...) in patch 2, I'm just not a fan. You're the author, you got my OK for your method even though it's not my personal preference. John
On 06/12/2017 05:57 PM, Michal Privoznik wrote:
Yet again, some corner cases, nothing critical. But it is certainly nice to fix them regardless.
Michal Privoznik (3): qemuDomainBuildNamespace: Clean up temp files qemuDomainGetPreservedMounts: Prune nested mount points qemuDomainGetPreservedMounts: Fix suffixes for corner cases
src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)
Thanks John for the review. I've pushed these. Michal
participants (2)
-
John Ferlan -
Michal Privoznik