[libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing
Commit id 'f1f68ca33' added code to remove the directory paths for auto-generated sockets, but that code could be called before the paths were created resulting in generating error messages from virFileDeleteTree indicating that the file doesn't exist. So just add a check before attemping the directory delete for existence. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ce2c70c..b55eb52 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver, ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->libDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath); ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->channelTargetDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath); -- 2.1.0
On Tue, Sep 15, 2015 at 05:03:45PM -0400, John Ferlan wrote:
Commit id 'f1f68ca33' added code to remove the directory paths for auto-generated sockets, but that code could be called before the paths were created resulting in generating error messages from virFileDeleteTree indicating that the file doesn't exist. So just add a check before attemping the directory delete for existence.
Oh, I haven't checked that virFileDeleteTree() causes an error. Since we don't want "silent" versions functions (even though we have many :| ), ACK to this. No race should happen, and if it does; no biggie.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ce2c70c..b55eb52 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->libDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath);
ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->channelTargetDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath);
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 15.09.2015 23:03, John Ferlan wrote:
Commit id 'f1f68ca33' added code to remove the directory paths for auto-generated sockets, but that code could be called before the paths were created resulting in generating error messages from virFileDeleteTree indicating that the file doesn't exist. So just add a check before attemping the directory delete for existence.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ce2c70c..b55eb52 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->libDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath);
ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->channelTargetDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath);
Okay, so this is needed so that we don't taint logs with useless error messages. ACK to the idea. But what what about moving virFileExists into virFileDeleteTree? The reason for that would be that I like functions which deals with dummy arguments. In this case it would be: virFileDeleteTree("/some/nonexistent/path"); virFileDeleteTree(NULL); But if you dislike the idea, ACK to this patch then. There's nothing wrong with it. Michal
On 09/16/2015 09:05 AM, Michal Privoznik wrote:
On 15.09.2015 23:03, John Ferlan wrote:
Commit id 'f1f68ca33' added code to remove the directory paths for auto-generated sockets, but that code could be called before the paths were created resulting in generating error messages from virFileDeleteTree indicating that the file doesn't exist. So just add a check before attemping the directory delete for existence.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ce2c70c..b55eb52 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->libDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath);
ignore_value(virAsprintf(&tmppath, "%s/domain-%s", cfg->channelTargetDir, vm->def->name)); - if (tmppath) + if (tmppath && virFileExists(tmppath)) virFileDeleteTree(tmppath); VIR_FREE(tmppath);
Okay, so this is needed so that we don't taint logs with useless error messages. ACK to the idea. But what what about moving virFileExists into virFileDeleteTree? The reason for that would be that I like functions which deals with dummy arguments. In this case it would be:
virFileDeleteTree("/some/nonexistent/path"); virFileDeleteTree(NULL);
But if you dislike the idea, ACK to this patch then. There's nothing wrong with it.
Correct with your assertion - I saw the messages while debugging something else (error validating something during qemu_command creation): 2015-09-15 14:18:40.197+0000: 28838: error : virFileDeleteTree:945 : Cannot open dir '/var/lib/libvirt/qemu/domain-test': No such file or directory 2015-09-15 14:18:40.197+0000: 28838: error : virFileDeleteTree:945 : Cannot open dir '/var/lib/libvirt/qemu/channel/target/domain-test': No such file or directory and that was after the error I was expecting... Since it was 'new' I knew it was a recent change... Anyway, I took the chisel approach rather than the sledgehammer, but I see your point. Up to this point it seems the function was primarily called from *test.c modules and those that weren't sure if 'path' could be NULL prior to call would always check. I'll post a different patch with the check in virFileDeleteTree() and of course remove the NULL checks in the *test.c modules. John
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik