[libvirt] [PATCH v2 0/3] Some additional checks for virDomainRename
So after some discussion to my original approach, this is v2. Michal Privoznik (3): virHashAddOrUpdateEntry: Tunr @new_name into void * virHashAddEntry: Report error on duplicate key qemuDomainRename: Explicitly check if domain is renaming to itself src/qemu/qemu_driver.c | 6 ++++++ src/util/virhash.c | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) -- 2.4.6
In 9190f0b0 we've tried to fix an OOM. And boy, was that fix successful. But back then, the hash table implementation worked strictly over string keys, which is not the case anymore. Hash table have this function keyCopy() which returns void *. Therefore a local variable that is temporarily holding the intermediate return value from that function should be void * too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virhash.c b/src/util/virhash.c index 3cfcc69..77196c9 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -334,7 +334,7 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name, { size_t key, len = 0; virHashEntryPtr entry; - char *new_name; + void *new_name; if ((table == NULL) || (name == NULL)) return -1; -- 2.4.6
On Mon, Aug 17, 2015 at 11:16:27PM +0200, Michal Privoznik wrote: s/Tunr/Turn/ in $SUBJ
In 9190f0b0 we've tried to fix an OOM. And boy, was that fix successful. But back then, the hash table implementation worked strictly over string keys, which is not the case anymore. Hash table have this function keyCopy() which returns void *. Therefore a local variable that is temporarily holding the intermediate return value from that function should be void * too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virhash.c b/src/util/virhash.c index 3cfcc69..77196c9 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -334,7 +334,7 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name, { size_t key, len = 0; virHashEntryPtr entry; - char *new_name; + void *new_name;
if ((table == NULL) || (name == NULL)) return -1; -- 2.4.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This function, when fails the error message is reported only in some cases (e.g. OOM) but in some it's not (e.g. duplicate key). This fact is painful and we should either not report error at all or report the error in all possible cases. I vote for the latter. Unfortunately, since the key may be an arbitrary value (not necessarily a string) we can't report it in the error message. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhash.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virhash.c b/src/util/virhash.c index 77196c9..bc90c44 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -353,6 +353,8 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name, entry->payload = userdata; return 0; } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Duplicate key")); return -1; } } -- 2.4.6
On Mon, Aug 17, 2015 at 11:16:28PM +0200, Michal Privoznik wrote:
This function, when fails the error message is reported only in some cases (e.g. OOM) but in some it's not (e.g. duplicate key).
The sentence could be reworded.
This fact is painful and we should either not report error at all or report the error in all possible cases. I vote for the latter. Unfortunately, since the key may be an arbitrary value (not necessarily a string) we can't report it in the error message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhash.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/virhash.c b/src/util/virhash.c index 77196c9..bc90c44 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -353,6 +353,8 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name, entry->payload = userdata; return 0; } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Duplicate key")); return -1; } } -- 2.4.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
It may happen that user (mistakenly) wants to rename a domain to itself. Which is no renaming at all. We should reject that with some meaningful error message. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4c05324..083e4ff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19937,6 +19937,12 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; } + if (STREQ(vm->def->name, new_name)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't rename domain to itself")); + goto cleanup; + } + if (VIR_STRDUP(new_dom_name, new_name) < 0) goto endjob; -- 2.4.6
On Mon, Aug 17, 2015 at 11:16:29PM +0200, Michal Privoznik wrote:
It may happen that user (mistakenly) wants to rename a domain to itself. Which is no renaming at all. We should reject that with some meaningful error message.
Or just return 0 :) (that way it's a feature)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4c05324..083e4ff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19937,6 +19937,12 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; }
+ if (STREQ(vm->def->name, new_name)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't rename domain to itself")); + goto cleanup; + } + if (VIR_STRDUP(new_dom_name, new_name) < 0) goto endjob;
-- 2.4.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Martin Kletzander -
Michal Privoznik