[libvirt] [PATCH] LXC fix rc handling in lxcVmStart
Hi, In lxcVmStart we have to set rc = -1 before jumping to the cleanup code when a local procedure call fails and the rc is overwritten, however, some codes don't observe the rule and so invalid cleanups are likely to happen. A simple solution for the defect is to add rc = -1 before every jumping if needed, however, IMHO, we first should not reuse rc for another purpose rather than the return value of lxcVmStart. Instead the patch introduces yet another variable for storing only the return value of the local procedure calls. By doing so, we don't need to care about resetting rc anymore. Thanks, ozaki-r
From c3eb2cfa1280b220d5b92ad2ad8bbf6cf6ca0a04 Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Tue, 13 Oct 2009 00:36:41 +0900 Subject: [PATCH] LXC fix rc handling in lxcVmStart
* src/lxc/lxc_driver.c: don't reuse rc for local procedure calls --- src/lxc/lxc_driver.c | 15 ++++++--------- 1 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ef0d368..0b614e3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1117,7 +1117,7 @@ static int lxcVmStart(virConnectPtr conn, lxc_driver_t * driver, virDomainObjPtr vm) { - int rc = -1; + int rc = -1, r; unsigned int i; int parentTty; char *parentTtyPath = NULL; @@ -1126,8 +1126,8 @@ static int lxcVmStart(virConnectPtr conn, unsigned int nveths = 0; char **veths = NULL; - if ((rc = virFileMakePath(driver->logDir)) < 0) { - virReportSystemError(conn, rc, + if ((r = virFileMakePath(driver->logDir)) < 0) { + virReportSystemError(conn, r, _("cannot create log directory '%s'"), driver->logDir); return -1; @@ -1157,10 +1157,8 @@ static int lxcVmStart(virConnectPtr conn, goto cleanup; /* Persist the live configuration now we have veth & tty info */ - if (virDomainSaveConfig(conn, driver->stateDir, vm->def) < 0) { - rc = -1; + if (virDomainSaveConfig(conn, driver->stateDir, vm->def) < 0) goto cleanup; - } if ((logfd = open(logfile, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR|S_IWUSR)) < 0) { @@ -1183,11 +1181,10 @@ static int lxcVmStart(virConnectPtr conn, goto cleanup; /* And get its pid */ - if ((rc = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) != 0) { - virReportSystemError(conn, rc, + if ((r = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) != 0) { + virReportSystemError(conn, r, _("Failed to read pid file %s/%s.pid"), driver->stateDir, vm->def->name); - rc = -1; goto cleanup; } -- 1.6.2.5
Ryota Ozaki wrote:
Hi,
In lxcVmStart we have to set rc = -1 before jumping to the cleanup code when a local procedure call fails and the rc is overwritten, however, some codes don't observe the rule and so invalid cleanups are likely to happen.
A simple solution for the defect is to add rc = -1 before every jumping if needed, however, IMHO, we first should not reuse rc for another purpose rather than the return value of lxcVmStart. Instead the patch introduces yet another variable for storing only the return value of the local procedure calls. By doing so, we don't need to care about resetting rc anymore.
Yes, that's a much better way to do it. ACK -- Chris Lalancette
On Tue, Oct 13, 2009 at 10:57:13AM +0900, Ryota Ozaki wrote:
Hi,
In lxcVmStart we have to set rc = -1 before jumping to the cleanup code when a local procedure call fails and the rc is overwritten, however, some codes don't observe the rule and so invalid cleanups are likely to happen.
A simple solution for the defect is to add rc = -1 before every jumping if needed, however, IMHO, we first should not reuse rc for another purpose rather than the return value of lxcVmStart. Instead the patch introduces yet another variable for storing only the return value of the local procedure calls. By doing so, we don't need to care about resetting rc anymore.
ACK, this is a good idea. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Oct 13, 2009 at 10:57:13AM +0900, Ryota Ozaki wrote:
Hi,
In lxcVmStart we have to set rc = -1 before jumping to the cleanup code when a local procedure call fails and the rc is overwritten, however, some codes don't observe the rule and so invalid cleanups are likely to happen.
A simple solution for the defect is to add rc = -1 before every jumping if needed, however, IMHO, we first should not reuse rc for another purpose rather than the return value of lxcVmStart. Instead the patch introduces yet another variable for storing only the return value of the local procedure calls. By doing so, we don't need to care about resetting rc anymore.
Makes sense, since it's a cleanup/bug fix I commited this now, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Ryota Ozaki