[libvirt] [PATCH] qemuDomainRename: Report error if adding domain to domainObj list fails
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no error is reported: virsh # domrename dummy dummy error: An error occurred, but the cause is unknown Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3683591..8e365bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19950,8 +19950,11 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; } - if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) + if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("could not add new name into internal list of domains")); goto endjob; + } if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) goto rollback; -- 2.4.6
On Fri, Aug 14, 2015 at 03:40:16PM +0200, Michal Privoznik wrote:
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no
Well, the problem here is that the first step can fail for 2 reasons. One of them is that the domain already exists, the second one is an OOM error. What's even worse is that the OOM error gets reported, but the duplicate key error doesn't. There should be either a hint for the user that domain with such name already exists or that should be checked right before. The second variant would not be race-free, but there would be only a small window of opportunity left. The con would be that you won't report "already exists" error when there's an OOM error already. Checking for that error seems too weird, it would be more worth fixing the has table implementation we have :)
error is reported:
virsh # domrename dummy dummy error: An error occurred, but the cause is unknown
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3683591..8e365bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19950,8 +19950,11 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; }
- if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) + if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("could not add new name into internal list of domains")); goto endjob; + }
if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) goto rollback; -- 2.4.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 15.08.2015 16:26, Martin Kletzander wrote:
On Fri, Aug 14, 2015 at 03:40:16PM +0200, Michal Privoznik wrote:
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no
Well, the problem here is that the first step can fail for 2 reasons. One of them is that the domain already exists, the second one is an OOM error. What's even worse is that the OOM error gets reported, but the duplicate key error doesn't.
There should be either a hint for the user that domain with such name already exists or that should be checked right before. The second variant would not be race-free, but there would be only a small window of opportunity left. The con would be that you won't report "already exists" error when there's an OOM error already. Checking for that error seems too weird, it would be more worth fixing the has table implementation we have :)
Hm.. I agree that the partial error reporting is bad and definitely calls for fixing. But I think it's gonna be tricky. I mean, we can't emit a generic enough error message. "Duplicate key $key" would not be much helpful for users since hash tables are used not only in virDomainObjList. But maybe it would be better than nothing. As a separate patch, we can reject the same name. Okay. Michal
On Mon, Aug 17, 2015 at 08:39:29PM +0200, Michal Privoznik wrote:
On 15.08.2015 16:26, Martin Kletzander wrote:
On Fri, Aug 14, 2015 at 03:40:16PM +0200, Michal Privoznik wrote:
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no
Well, the problem here is that the first step can fail for 2 reasons. One of them is that the domain already exists, the second one is an OOM error. What's even worse is that the OOM error gets reported, but the duplicate key error doesn't.
There should be either a hint for the user that domain with such name already exists or that should be checked right before. The second variant would not be race-free, but there would be only a small window of opportunity left. The con would be that you won't report "already exists" error when there's an OOM error already. Checking for that error seems too weird, it would be more worth fixing the has table implementation we have :)
Hm.. I agree that the partial error reporting is bad and definitely calls for fixing. But I think it's gonna be tricky. I mean, we can't emit a generic enough error message. "Duplicate key $key" would not be much helpful for users since hash tables are used not only in virDomainObjList. But maybe it would be better than nothing.
As a separate patch, we can reject the same name. Okay.
Yeah, it's enough for me to add a check for same name and that it doesn't already exists.
Michal
On Fri, Aug 14, 2015 at 03:40:16PM +0200, Michal Privoznik wrote:
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no error is reported:
virsh # domrename dummy dummy error: An error occurred, but the cause is unknown
ACK series.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3683591..8e365bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19950,8 +19950,11 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; }
- if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) + if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("could not add new name into internal list of domains")); goto endjob; + }
if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) goto rollback; -- 2.4.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 17.08.2015 23:27, Martin Kletzander wrote:
On Fri, Aug 14, 2015 at 03:40:16PM +0200, Michal Privoznik wrote:
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no error is reported:
virsh # domrename dummy dummy error: An error occurred, but the cause is unknown
ACK series.
I believe this was meant to the v2. So I've pushed that one. Thanks! Michal
On Mon, Aug 17, 2015 at 11:45:09PM +0200, Michal Privoznik wrote:
On 17.08.2015 23:27, Martin Kletzander wrote:
On Fri, Aug 14, 2015 at 03:40:16PM +0200, Michal Privoznik wrote:
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no error is reported:
virsh # domrename dummy dummy error: An error occurred, but the cause is unknown
ACK series.
I believe this was meant to the v2. So I've pushed that one. Thanks!
Yes, you got it ;)
On Mon, Aug 17, 2015 at 02:27:57PM -0700, Martin Kletzander wrote:
On Fri, Aug 14, 2015 at 03:40:16PM +0200, Michal Privoznik wrote:
So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no error is reported:
virsh # domrename dummy dummy error: An error occurred, but the cause is unknown
ACK series.
Sorry for that, disregard this mail, I was missing cover letter in one of your series that was right below this mail.
participants (2)
-
Martin Kletzander -
Michal Privoznik