12:25 p.m.
Both may return a positive value when they fail. We should check
if the value is not zero instead of checking if it's negative.
lxcContainerRenameAndEnableInterfaces is expected to return a negative
value on a failure, so the patch changes the return value to -1
if vethInterfaceUpOrDown fails.
Note that this patch may be related to the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=607496 .
It would not fix the bug, but would unveil what happens.
---
src/lxc/lxc_container.c | 7 ++++++-
src/lxc/lxc_controller.c | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..c77d262 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -273,8 +273,13 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int
nveths,
}
/* enable lo device only if there were other net devices */
- if (veths)
+ if (veths) {
rc = vethInterfaceUpOrDown("lo", 1);
+ if (0 != rc) {
+ VIR_ERROR(_("Failed to enable lo (%d)"), rc);
+ rc = -1;
+ }
+ }
error_out:
VIR_FREE(newname);
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d8b7bc7..9829a69 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -477,7 +477,7 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (moveInterfaceToNetNs(veths[i], container) < 0) {
+ if (moveInterfaceToNetNs(veths[i], container) != 0) {
lxcError(VIR_ERR_INTERNAL_ERROR,
_("Failed to move interface %s to ns %d"),
veths[i], container);
--
1.6.6.1
12:58 p.m.
New subject: [libvirt] [PATCH] lxc: Fix return value handlings of vethInterfaceUpOrDown and moveInterfaceToNetNs
On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
Both may return a positive value when they fail. We should check
if the value is not zero instead of checking if it's negative.
I notice that lxcSetupInterfaces has a comment saying that it returns
-1 on failure, but it actually just passes on what is returned by
vethInterfaceUpOrDown.
Would you be willing to consider instead just changing
vethInterfaceUpOrDown and moveInterfaceToNetNs to return -1 in all error
cases (and checking the return for < 0), rather than grabbing the exit
code of the exec'ed command? None of the callers do anything with that
extra information anyway, and it would help to make the return values
more consistent (which makes it easier to catch bugs like this, or
eliminates them altogether ;-)
(I was recently bitten by a similar bug...)
lxcContainerRenameAndEnableInterfaces is expected to return a
negative
value on a failure, so the patch changes the return value to -1
if vethInterfaceUpOrDown fails.
Note that this patch may be related to the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=607496 .
It would not fix the bug, but would unveil what happens.
---
src/lxc/lxc_container.c | 7 ++++++-
src/lxc/lxc_controller.c | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..c77d262 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -273,8 +273,13 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int
nveths,
}
/* enable lo device only if there were other net devices */
- if (veths)
+ if (veths) {
rc = vethInterfaceUpOrDown("lo", 1);
+ if (0 != rc) {
+ VIR_ERROR(_("Failed to enable lo (%d)"), rc);
+ rc = -1;
+ }
+ }
error_out:
VIR_FREE(newname);
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d8b7bc7..9829a69 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -477,7 +477,7 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i< nveths ; i++)
- if (moveInterfaceToNetNs(veths[i], container)< 0) {
+ if (moveInterfaceToNetNs(veths[i], container) != 0) {
lxcError(VIR_ERR_INTERNAL_ERROR,
_("Failed to move interface %s to ns %d"),
veths[i], container);
9:44 a.m.
Hi Laine,
On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine(a)laine.org> wrote:
On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>
> Both may return a positive value when they fail. We should check
> if the value is not zero instead of checking if it's negative.
I notice that lxcSetupInterfaces has a comment saying that it returns -1 on
failure, but it actually just passes on what is returned by
vethInterfaceUpOrDown.
Oh, I didn't know that.
Additionally, I found that other functions, e.g., setMacAddr, are also handled
with the wrong way. And also handling return values with virReportSystemError
is also wrong because it expects an errno, not an exit code. We have to fix
all of them ;-<
Would you be willing to consider instead just changing vethInterfaceUpOrDown
and moveInterfaceToNetNs to return -1 in all error cases (and checking the
return for < 0), rather than grabbing the exit code of the exec'ed command?
None of the callers do anything with that extra information anyway, and it
would help to make the return values more consistent (which makes it easier
to catch bugs like this, or eliminates them altogether ;-)
Yeah, I'm also a bit annoying with the return values. Hmm, but we now show error
messages with the return values outside the functions. Without that, we have to
show the error message in the functions or some other place, otherwise we lose
useful information of errors. It seems not good idea.
One option is to let virRun show an error message by passing NULL to the second
argument (status) of it, like brSetEnableSTP in util/bridge.c, and
always return -1
on a failure. It'd satisfy what you suggest.
Honestly said, I cannot decide. Anyone has any suggestions on that?
Thanks,
ozaki-r
(I was recently bitten by a similar bug...)
> lxcContainerRenameAndEnableInterfaces is expected to return a negative
> value on a failure, so the patch changes the return value to -1
> if vethInterfaceUpOrDown fails.
>
> Note that this patch may be related to the bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=607496 .
> It would not fix the bug, but would unveil what happens.
> ---
> src/lxc/lxc_container.c | 7 ++++++-
> src/lxc/lxc_controller.c | 2 +-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 4371dba..c77d262 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -273,8 +273,13 @@ static int
> lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
> }
>
> /* enable lo device only if there were other net devices */
> - if (veths)
> + if (veths) {
> rc = vethInterfaceUpOrDown("lo", 1);
> + if (0 != rc) {
> + VIR_ERROR(_("Failed to enable lo (%d)"), rc);
> + rc = -1;
> + }
> + }
>
> error_out:
> VIR_FREE(newname);
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index d8b7bc7..9829a69 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -477,7 +477,7 @@ static int lxcControllerMoveInterfaces(unsigned int
> nveths,
> {
> unsigned int i;
> for (i = 0 ; i< nveths ; i++)
> - if (moveInterfaceToNetNs(veths[i], container)< 0) {
> + if (moveInterfaceToNetNs(veths[i], container) != 0) {
> lxcError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to move interface %s to ns %d"),
> veths[i], container);
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:25 p.m.
On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota(a)gmail.com> wrote:
Hi Laine,
On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine(a)laine.org> wrote:
> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>>
>> Both may return a positive value when they fail. We should check
>> if the value is not zero instead of checking if it's negative.
>
> I notice that lxcSetupInterfaces has a comment saying that it returns -1 on
> failure, but it actually just passes on what is returned by
> vethInterfaceUpOrDown.
Oh, I didn't know that.
Additionally, I found that other functions, e.g., setMacAddr, are also handled
with the wrong way. And also handling return values with virReportSystemError
is also wrong because it expects an errno, not an exit code. We have to fix
all of them ;-<
>
> Would you be willing to consider instead just changing vethInterfaceUpOrDown
> and moveInterfaceToNetNs to return -1 in all error cases (and checking the
> return for < 0), rather than grabbing the exit code of the exec'ed command?
> None of the callers do anything with that extra information anyway, and it
> would help to make the return values more consistent (which makes it easier
> to catch bugs like this, or eliminates them altogether ;-)
Yeah, I'm also a bit annoying with the return values. Hmm, but we now show error
messages with the return values outside the functions. Without that, we have to
show the error message in the functions or some other place, otherwise we lose
useful information of errors. It seems not good idea.
One option is to let virRun show an error message by passing NULL to the second
argument (status) of it, like brSetEnableSTP in util/bridge.c, and
always return -1
on a failure. It'd satisfy what you suggest.
Honestly said, I cannot decide. Anyone has any suggestions on that?
Anyway, I've created a patch following Laine's suggestion. Certainly it
looks pretty cleaner than so far ;-)
ozaki-r
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..5671852 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -244,29 +244,24 @@ static int lxcContainerWaitForContinue(int control)
static int lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
char **veths)
{
- int rc = 0;
+ int rc = -1;
unsigned int i;
char *newname = NULL;
for (i = 0 ; i < nveths ; i++) {
- rc = virAsprintf(&newname, "eth%d", i);
- if (rc < 0)
+ if (virAsprintf(&newname, "eth%d", i) < 0)
goto error_out;
DEBUG("Renaming %s to %s", veths[i], newname);
- rc = setInterfaceName(veths[i], newname);
- if (0 != rc) {
- VIR_ERROR(_("Failed to rename %s to %s (%d)"),
- veths[i], newname, rc);
- rc = -1;
+ if (setInterfaceName(veths[i], newname) < 0) {
+ VIR_ERROR(_("Failed to rename %s to %s"),
+ veths[i], newname);
goto error_out;
}
DEBUG("Enabling %s", newname);
- rc = vethInterfaceUpOrDown(newname, 1);
- if (0 != rc) {
- VIR_ERROR(_("Failed to enable %s (%d)"), newname, rc);
- rc = -1;
+ if (vethInterfaceUpOrDown(newname, 1) < 0) {
+ VIR_ERROR(_("Failed to enable %s"), newname);
goto error_out;
}
VIR_FREE(newname);
@@ -274,8 +269,12 @@ static int
lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
/* enable lo device only if there were other net devices */
if (veths)
- rc = vethInterfaceUpOrDown("lo", 1);
+ if (vethInterfaceUpOrDown("lo", 1) < 0) {
+ VIR_ERROR("%s", _("Failed to enable lo"));
+ goto error_out;
+ }
+ rc = 0;
error_out:
VIR_FREE(newname);
return rc;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 462bc9c..773ee2e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -859,9 +859,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
strcpy(parentVeth, def->nets[i]->ifname);
}
DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- if (0 != (rc = vethCreate(parentVeth, PATH_MAX,
containerVeth, PATH_MAX))) {
+ if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0) {
lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to create veth device pair: %d"), rc);
+ _("Failed to create veth device pair: %s, %s"),
+ parentVeth, containerVeth);
goto error_exit;
}
if (NULL == def->nets[i]->ifname) {
@@ -885,10 +886,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
- if (0 != (rc = setMacAddr(containerVeth, macaddr))) {
- virReportSystemError(rc,
- _("Failed to set %s to %s"),
- macaddr, containerVeth);
+ if (setMacAddr(containerVeth, macaddr) < 0) {
+ lxcError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set %s to %s"),
+ macaddr, containerVeth);
goto error_exit;
}
}
@@ -900,10 +901,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
goto error_exit;
}
- if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
- virReportSystemError(rc,
- _("Failed to enable %s device"),
- parentVeth);
+ if (vethInterfaceUpOrDown(parentVeth, 1) < 0) {
+ lxcError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to enable %s device"),
+ parentVeth);
goto error_exit;
}
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa..39c3a50 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -76,16 +76,13 @@ static int getFreeVethName(char *veth, int maxLen,
int startDev)
int vethCreate(char* veth1, int veth1MaxLen,
char* veth2, int veth2MaxLen)
{
- int rc = -1;
const char *argv[] = {
"ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
};
- int cmdResult;
int vethDev = 0;
- if ((NULL == veth1) || (NULL == veth2)) {
- goto error_out;
- }
+ if ((NULL == veth1) || (NULL == veth2))
+ return -1;
DEBUG("veth1: %s veth2: %s", veth1, veth2);
@@ -102,14 +99,10 @@ int vethCreate(char* veth1, int veth1MaxLen,
}
DEBUG("veth1: %s veth2: %s", veth1, veth2);
- rc = virRun(argv, &cmdResult);
-
- if (0 == rc) {
- rc = cmdResult;
- }
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -125,24 +118,17 @@ error_out:
*/
int vethDelete(const char *veth)
{
- int rc = -1;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
- int cmdResult;
- if (NULL == veth) {
- goto error_out;
- }
+ if (NULL == veth)
+ return -1;
DEBUG("veth: %s", veth);
- rc = virRun(argv, &cmdResult);
-
- if (0 == rc) {
- rc = cmdResult;
- }
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -157,27 +143,20 @@ error_out:
*/
int vethInterfaceUpOrDown(const char* veth, int upOrDown)
{
- int rc = -1;
const char *argv[] = {"ifconfig", veth, NULL, NULL};
- int cmdResult;
- if (NULL == veth) {
- goto error_out;
- }
+ if (NULL == veth)
+ return -1;
if (0 == upOrDown)
argv[2] = "down";
else
argv[2] = "up";
- rc = virRun(argv, &cmdResult);
-
- if (0 == rc) {
- rc = cmdResult;
- }
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -198,19 +177,17 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
const char *argv[] = {
"ip", "link", "set", iface, "netns",
NULL, NULL
};
- int cmdResult;
- if (NULL == iface) {
+ if (NULL == iface)
goto error_out;
- }
if (virAsprintf(&pid, "%d", pidInNs) == -1)
goto error_out;
argv[5] = pid;
- rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ rc = virRun(argv, NULL);
+ if (rc < 0)
+ rc = -1;
error_out:
VIR_FREE(pid);
@@ -230,22 +207,17 @@ error_out:
*/
int setMacAddr(const char* iface, const char* macaddr)
{
- int rc = -1;
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
- int cmdResult;
- if (NULL == iface) {
- goto error_out;
- }
+ if (NULL == iface)
+ return -1;
- rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
/**
@@ -261,20 +233,15 @@ error_out:
*/
int setInterfaceName(const char* iface, const char* new)
{
- int rc = -1;
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
- int cmdResult;
- if (NULL == iface || NULL == new) {
- goto error_out;
- }
+ if (NULL == iface || NULL == new)
+ return -1;
- rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (virRun(argv, NULL) < 0)
+ return -1;
-error_out:
- return rc;
+ return 0;
}
4:51 a.m.
On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki
<ozaki.ryota(a)gmail.com> wrote:
> Hi Laine,
>
> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine(a)laine.org> wrote:
>> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>>>
>>> Both may return a positive value when they fail. We should check
>>> if the value is not zero instead of checking if it's negative.
>>
>> I notice that lxcSetupInterfaces has a comment saying that it returns -1 on
>> failure, but it actually just passes on what is returned by
>> vethInterfaceUpOrDown.
>
> Oh, I didn't know that.
>
> Additionally, I found that other functions, e.g., setMacAddr, are also handled
> with the wrong way. And also handling return values with virReportSystemError
> is also wrong because it expects an errno, not an exit code. We have to fix
> all of them ;-<
>
>>
>> Would you be willing to consider instead just changing vethInterfaceUpOrDown
>> and moveInterfaceToNetNs to return -1 in all error cases (and checking the
>> return for < 0), rather than grabbing the exit code of the exec'ed
command?
>> None of the callers do anything with that extra information anyway, and it
>> would help to make the return values more consistent (which makes it easier
>> to catch bugs like this, or eliminates them altogether ;-)
>
> Yeah, I'm also a bit annoying with the return values. Hmm, but we now show
error
> messages with the return values outside the functions. Without that, we have to
> show the error message in the functions or some other place, otherwise we lose
> useful information of errors. It seems not good idea.
>
> One option is to let virRun show an error message by passing NULL to the second
> argument (status) of it, like brSetEnableSTP in util/bridge.c, and
> always return -1
> on a failure. It'd satisfy what you suggest.
>
> Honestly said, I cannot decide. Anyone has any suggestions on that?
You could just change
return cmdResult
to
return -cmdResult;
That would still let you give the error code, while also keeping the value
< 0
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
7:31 a.m.
On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange <berrange(a)redhat.com> wrote:
On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
> On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota(a)gmail.com> wrote:
> > Hi Laine,
> >
> > On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine(a)laine.org> wrote:
> >> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
> >>>
> >>> Both may return a positive value when they fail. We should check
> >>> if the value is not zero instead of checking if it's negative.
> >>
> >> I notice that lxcSetupInterfaces has a comment saying that it returns -1
on
> >> failure, but it actually just passes on what is returned by
> >> vethInterfaceUpOrDown.
> >
> > Oh, I didn't know that.
> >
> > Additionally, I found that other functions, e.g., setMacAddr, are also handled
> > with the wrong way. And also handling return values with virReportSystemError
> > is also wrong because it expects an errno, not an exit code. We have to fix
> > all of them ;-<
> >
> >>
> >> Would you be willing to consider instead just changing
vethInterfaceUpOrDown
> >> and moveInterfaceToNetNs to return -1 in all error cases (and checking the
> >> return for < 0), rather than grabbing the exit code of the exec'ed
command?
> >> None of the callers do anything with that extra information anyway, and it
> >> would help to make the return values more consistent (which makes it easier
> >> to catch bugs like this, or eliminates them altogether ;-)
> >
> > Yeah, I'm also a bit annoying with the return values. Hmm, but we now show
error
> > messages with the return values outside the functions. Without that, we have to
> > show the error message in the functions or some other place, otherwise we lose
> > useful information of errors. It seems not good idea.
> >
> > One option is to let virRun show an error message by passing NULL to the second
> > argument (status) of it, like brSetEnableSTP in util/bridge.c, and
> > always return -1
> > on a failure. It'd satisfy what you suggest.
> >
> > Honestly said, I cannot decide. Anyone has any suggestions on that?
You could just change
return cmdResult
to
return -cmdResult;
That would still let you give the error code, while also keeping the value
< 0
It looks better than mine ;-) I'll rewrite my patch in such a way.
Laine, is it ok for you too?
Thanks,
ozaki-r
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:42 a.m.
On Mon, Jul 26, 2010 at 09:31:21PM +0900, Ryota Ozaki wrote:
On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange
<berrange(a)redhat.com> wrote:
> On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
>> On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota(a)gmail.com>
wrote:
>> > Hi Laine,
>> >
>> > On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine(a)laine.org>
wrote:
>> >> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>> >>>
>> >>> Both may return a positive value when they fail. We should check
>> >>> if the value is not zero instead of checking if it's negative.
>> >>
>> >> I notice that lxcSetupInterfaces has a comment saying that it returns
-1 on
>> >> failure, but it actually just passes on what is returned by
>> >> vethInterfaceUpOrDown.
>> >
>> > Oh, I didn't know that.
>> >
>> > Additionally, I found that other functions, e.g., setMacAddr, are also
handled
>> > with the wrong way. And also handling return values with
virReportSystemError
>> > is also wrong because it expects an errno, not an exit code. We have to
fix
>> > all of them ;-<
>> >
>> >>
>> >> Would you be willing to consider instead just changing
vethInterfaceUpOrDown
>> >> and moveInterfaceToNetNs to return -1 in all error cases (and checking
the
>> >> return for < 0), rather than grabbing the exit code of the
exec'ed command?
>> >> None of the callers do anything with that extra information anyway, and
it
>> >> would help to make the return values more consistent (which makes it
easier
>> >> to catch bugs like this, or eliminates them altogether ;-)
>> >
>> > Yeah, I'm also a bit annoying with the return values. Hmm, but we now
show error
>> > messages with the return values outside the functions. Without that, we
have to
>> > show the error message in the functions or some other place, otherwise we
lose
>> > useful information of errors. It seems not good idea.
>> >
>> > One option is to let virRun show an error message by passing NULL to the
second
>> > argument (status) of it, like brSetEnableSTP in util/bridge.c, and
>> > always return -1
>> > on a failure. It'd satisfy what you suggest.
>> >
>> > Honestly said, I cannot decide. Anyone has any suggestions on that?
>
> You could just change
>
> return cmdResult
>
> to
>
> return -cmdResult;
>
> That would still let you give the error code, while also keeping the value
> < 0
It looks better than mine ;-) I'll rewrite my patch in such a way.
Actually the other third option is to just call virReportError from
within the veth functions themselves, instead of logging errors in
the caller. This might be a better long term approach.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:37 a.m.
On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
On Mon, Jul 26, 2010 at 6:51 PM, Daniel P.
Berrange<berrange(a)redhat.com> wrote:
> On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
>> On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki<ozaki.ryota(a)gmail.com>
wrote:
>>> Hi Laine,
>>>
>>> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump<laine(a)laine.org> wrote:
>>>> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>>>>> Both may return a positive value when they fail. We should check
>>>>> if the value is not zero instead of checking if it's negative.
>>>> I notice that lxcSetupInterfaces has a comment saying that it returns -1
on
>>>> failure, but it actually just passes on what is returned by
>>>> vethInterfaceUpOrDown.
>>> Oh, I didn't know that.
>>>
>>> Additionally, I found that other functions, e.g., setMacAddr, are also
handled
>>> with the wrong way. And also handling return values with
virReportSystemError
>>> is also wrong because it expects an errno, not an exit code. We have to fix
>>> all of them ;-<
>>>
>>>> Would you be willing to consider instead just changing
vethInterfaceUpOrDown
>>>> and moveInterfaceToNetNs to return -1 in all error cases (and checking
the
>>>> return for< 0), rather than grabbing the exit code of the exec'ed
command?
>>>> None of the callers do anything with that extra information anyway, and
it
>>>> would help to make the return values more consistent (which makes it
easier
>>>> to catch bugs like this, or eliminates them altogether ;-)
>>> Yeah, I'm also a bit annoying with the return values. Hmm, but we now
show error
>>> messages with the return values outside the functions. Without that, we have
to
>>> show the error message in the functions or some other place, otherwise we
lose
>>> useful information of errors. It seems not good idea.
>>>
>>> One option is to let virRun show an error message by passing NULL to the
second
>>> argument (status) of it, like brSetEnableSTP in util/bridge.c, and
>>> always return -1
>>> on a failure. It'd satisfy what you suggest.
>>>
>>> Honestly said, I cannot decide. Anyone has any suggestions on that?
> You could just change
>
> return cmdResult
>
> to
>
> return -cmdResult;
>
> That would still let you give the error code, while also keeping the value
> < 0
It looks better than mine ;-) I'll rewrite my patch in such a way.
Laine, is it ok for you too?
Doing that is fine when all possible failures in the function have an
associated errno. In the case of virRun'ing an external program that
could return a non-zero exit code, this is unfortunately not the case,
so those functions that call virRun will need to report the error
themselves and return -1.
When non-0 exits from the called program are all failures, the simplest
way to do it is, as you say, to just not pass a pointer to a resultcode
to virRun (as long as the error message reported by virRun is useful
enough - remember that it gives the name of the program being run, and
"virRun", but not the name of the calling function).
setMacAddr gives another example of a failure mode that breaks the
"return -errno" paradigm - it checks for one of the arguments being
NULL, and fails in that case. If it's important to maintain "-errno on
failure" in one of those cases, possibly examining the code will show
that the arg in question is never NULL, in which case you can mark it in
its prototype with ATTRIBUTE_NONNULL and just eliminate that failure
from the code.
It seems that in general, the -errno convention works better at lower
levels where all the failures are related to some system call failing,
but at some point higher in the call chain the possibility of failure
due to config, etc, comes in, there is no valid errno to describe a
problem, and then you need to start reporting the errors and returning -1.
7:58 a.m.
Hi Laine,
Hmm...I was eager to a conclusion. We need some more discussion to
a better way.
Nonetheless, I hope this fix and another which I found later to be
included in the next release that is expected soon. Is it OK for you,
Laine? Of course, I'll continue this discussion until we have a consensus.
ozaki-r
On Mon, Jul 26, 2010 at 11:37 PM, Laine Stump <laine(a)laine.org> wrote:
On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
>
> On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange<berrange(a)redhat.com>
> wrote:
>>
>> On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
>>>
>>> On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki<ozaki.ryota(a)gmail.com>
>>> wrote:
>>>>
>>>> Hi Laine,
>>>>
>>>> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump<laine(a)laine.org>
wrote:
>>>>>
>>>>> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>>>>>>
>>>>>> Both may return a positive value when they fail. We should check
>>>>>> if the value is not zero instead of checking if it's
negative.
>>>>>
>>>>> I notice that lxcSetupInterfaces has a comment saying that it
returns
>>>>> -1 on
>>>>> failure, but it actually just passes on what is returned by
>>>>> vethInterfaceUpOrDown.
>>>>
>>>> Oh, I didn't know that.
>>>>
>>>> Additionally, I found that other functions, e.g., setMacAddr, are also
>>>> handled
>>>> with the wrong way. And also handling return values with
>>>> virReportSystemError
>>>> is also wrong because it expects an errno, not an exit code. We have to
>>>> fix
>>>> all of them ;-<
>>>>
>>>>> Would you be willing to consider instead just changing
>>>>> vethInterfaceUpOrDown
>>>>> and moveInterfaceToNetNs to return -1 in all error cases (and
checking
>>>>> the
>>>>> return for< 0), rather than grabbing the exit code of the
exec'ed
>>>>> command?
>>>>> None of the callers do anything with that extra information anyway,
>>>>> and it
>>>>> would help to make the return values more consistent (which makes it
>>>>> easier
>>>>> to catch bugs like this, or eliminates them altogether ;-)
>>>>
>>>> Yeah, I'm also a bit annoying with the return values. Hmm, but we
now
>>>> show error
>>>> messages with the return values outside the functions. Without that, we
>>>> have to
>>>> show the error message in the functions or some other place, otherwise
>>>> we lose
>>>> useful information of errors. It seems not good idea.
>>>>
>>>> One option is to let virRun show an error message by passing NULL to
>>>> the second
>>>> argument (status) of it, like brSetEnableSTP in util/bridge.c, and
>>>> always return -1
>>>> on a failure. It'd satisfy what you suggest.
>>>>
>>>> Honestly said, I cannot decide. Anyone has any suggestions on that?
>>
>> You could just change
>>
>> return cmdResult
>>
>> to
>>
>> return -cmdResult;
>>
>> That would still let you give the error code, while also keeping the
>> value
>> < 0
>
> It looks better than mine ;-) I'll rewrite my patch in such a way.
>
> Laine, is it ok for you too?
>
Doing that is fine when all possible failures in the function have an
associated errno. In the case of virRun'ing an external program that could
return a non-zero exit code, this is unfortunately not the case, so those
functions that call virRun will need to report the error themselves and
return -1.
When non-0 exits from the called program are all failures, the simplest way
to do it is, as you say, to just not pass a pointer to a resultcode to
virRun (as long as the error message reported by virRun is useful enough -
remember that it gives the name of the program being run, and "virRun", but
not the name of the calling function).
setMacAddr gives another example of a failure mode that breaks the "return
-errno" paradigm - it checks for one of the arguments being NULL, and fails
in that case. If it's important to maintain "-errno on failure" in one of
those cases, possibly examining the code will show that the arg in question
is never NULL, in which case you can mark it in its prototype with
ATTRIBUTE_NONNULL and just eliminate that failure from the code.
It seems that in general, the -errno convention works better at lower levels
where all the failures are related to some system call failing, but at some
point higher in the call chain the possibility of failure due to config,
etc, comes in, there is no valid errno to describe a problem, and then you
need to start reporting the errors and returning -1.
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:11 a.m.
On Mon, Jul 26, 2010 at 11:37 PM, Laine Stump <laine(a)laine.org> wrote:
On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
>
> On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange<berrange(a)redhat.com>
> wrote:
>>
>> On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
>>>
>>> On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki<ozaki.ryota(a)gmail.com>
>>> wrote:
>>>>
>>>> Hi Laine,
>>>>
>>>> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump<laine(a)laine.org>
wrote:
>>>>>
>>>>> On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
>>>>>>
>>>>>> Both may return a positive value when they fail. We should check
>>>>>> if the value is not zero instead of checking if it's
negative.
>>>>>
>>>>> I notice that lxcSetupInterfaces has a comment saying that it
returns
>>>>> -1 on
>>>>> failure, but it actually just passes on what is returned by
>>>>> vethInterfaceUpOrDown.
>>>>
>>>> Oh, I didn't know that.
>>>>
>>>> Additionally, I found that other functions, e.g., setMacAddr, are also
>>>> handled
>>>> with the wrong way. And also handling return values with
>>>> virReportSystemError
>>>> is also wrong because it expects an errno, not an exit code. We have to
>>>> fix
>>>> all of them ;-<
>>>>
>>>>> Would you be willing to consider instead just changing
>>>>> vethInterfaceUpOrDown
>>>>> and moveInterfaceToNetNs to return -1 in all error cases (and
checking
>>>>> the
>>>>> return for< 0), rather than grabbing the exit code of the
exec'ed
>>>>> command?
>>>>> None of the callers do anything with that extra information anyway,
>>>>> and it
>>>>> would help to make the return values more consistent (which makes it
>>>>> easier
>>>>> to catch bugs like this, or eliminates them altogether ;-)
>>>>
>>>> Yeah, I'm also a bit annoying with the return values. Hmm, but we
now
>>>> show error
>>>> messages with the return values outside the functions. Without that, we
>>>> have to
>>>> show the error message in the functions or some other place, otherwise
>>>> we lose
>>>> useful information of errors. It seems not good idea.
>>>>
>>>> One option is to let virRun show an error message by passing NULL to
>>>> the second
>>>> argument (status) of it, like brSetEnableSTP in util/bridge.c, and
>>>> always return -1
>>>> on a failure. It'd satisfy what you suggest.
>>>>
>>>> Honestly said, I cannot decide. Anyone has any suggestions on that?
>>
>> You could just change
>>
>> return cmdResult
>>
>> to
>>
>> return -cmdResult;
>>
>> That would still let you give the error code, while also keeping the
>> value
>> < 0
>
> It looks better than mine ;-) I'll rewrite my patch in such a way.
>
> Laine, is it ok for you too?
>
Doing that is fine when all possible failures in the function have an
associated errno. In the case of virRun'ing an external program that could
return a non-zero exit code, this is unfortunately not the case, so those
functions that call virRun will need to report the error themselves and
return -1.
For veth.c all functions match the latter case while bridge.c has both.
If we don't take care about the consistency between veth.c and bridge.c,
we can focus on how to treat the latter case. (Ideally keeping the consistency
is better, but it seems difficult...)
When non-0 exits from the called program are all failures, the simplest way
to do it is, as you say, to just not pass a pointer to a resultcode to
virRun (as long as the error message reported by virRun is useful enough -
Yes.
remember that it gives the name of the program being run, and
"virRun", but
not the name of the calling function).
Agreed. That'll lose useful information for debugging. One option is
to re-report
an error in the calling function. It will lead reporting two messages,
but it should
be more useful than less reports.
One concern aobut virRun's error reporting is that it shows standard
errors through
DEBUG so by default it shows nothing while they are important for ifconfig
and ip commands because their error messages may be according to errno.
setMacAddr gives another example of a failure mode that breaks the "return
-errno" paradigm - it checks for one of the arguments being NULL, and fails
in that case. If it's important to maintain "-errno on failure" in one of
those cases, possibly examining the code will show that the arg in question
is never NULL, in which case you can mark it in its prototype with
ATTRIBUTE_NONNULL and just eliminate that failure from the code.
That seems good idea. One exception is virAsprintf in moveInterfaceToNetNs,
although we can simply use virReportOOMError() for it and return -ENOMEM.
It seems that in general, the -errno convention works better at lower levels
where all the failures are related to some system call failing, but at some
point higher in the call chain the possibility of failure due to config,
etc, comes in, there is no valid errno to describe a problem, and then you
need to start reporting the errors and returning -1.
Actually I had an experience that 'ip' command was not installed in a container
and I got an error not relating to errno. In the case, it was useful for me that
reporting errors in both a caller function and virRun.
I have now notified that mixing use of -errno and -1 is not good idea because
-1 is interpreted as -EPERM. (That's a reason why errnos and exit codes are
returned as is?)
ozaki-r
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:15 p.m.
On 07/27/2010 11:11 AM, Ryota Ozaki wrote:
On Mon, Jul 26, 2010 at 11:37 PM, Laine Stump<laine(a)laine.org>
wrote:
> On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
>> On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange<berrange(a)redhat.com>
>> wrote:
>>> You could just change
>>>
>>> return cmdResult
>>>
>>> to
>>>
>>> return -cmdResult;
>>>
>>> That would still let you give the error code, while also keeping the
>>> value
>>> < 0
>> It looks better than mine ;-) I'll rewrite my patch in such a way.
>>
>> Laine, is it ok for you too?
>>
> Doing that is fine when all possible failures in the function have an
> associated errno. In the case of virRun'ing an external program that could
> return a non-zero exit code, this is unfortunately not the case, so those
> functions that call virRun will need to report the error themselves and
> return -1.
For veth.c all functions match the latter case while bridge.c has both.
If we don't take care about the consistency between veth.c and bridge.c,
we can focus on how to treat the latter case. (Ideally keeping the consistency
is better, but it seems difficult...)
We can ignore bridge.c for now - there is a lot of code in libvirt that
doesn't follow the "-errno on failure" convention, so we should try to
be as narrow as possible. For this cleanup, I like the scope of just
veth.c and its direct callers.
Another thing I didn't notice when I wrote my first comment on this
thread is that most of the error conditions in the involved functions
are due to an external program failing, which will likely be due to the
program returning a non-0 code. However, that's not *always* the case,
so we can't really say that the function will always return a valid
"-exitcode", nor can we say it will always return a valid "-errno"
(and
as you point out, they can conflict).
> When non-0 exits from the called program are all failures, the
simplest way
> to do it is, as you say, to just not pass a pointer to a resultcode to
> virRun (as long as the error message reported by virRun is useful enough -
Yes.
> remember that it gives the name of the program being run, and "virRun",
but
> not the name of the calling function).
Agreed. That'll lose useful information for debugging. One option is
to re-report
an error in the calling function. It will lead reporting two messages,
but it should
be more useful than less reports.
One concern aobut virRun's error reporting is that it shows standard
errors through
DEBUG so by default it shows nothing while they are important for ifconfig
and ip commands because their error messages may be according to errno.
I also recall once in some other code letting virRun display the error
and the result was that someone else who got the error (which was really
unimportant) was handed a giant scary looking (and uninformative) error
message that took their attention away from the real problem, which was
elsewhere (I forget the details now, but you get the idea). It's
convenient, but if you want meaningful errors, I would recommend against
letting virRun report them.
I went through all the calls to all the functions listed in veth.h, and
I see that they are called from relatively few places, and the error
logs are almost always to the veth.c function that's being called, not
to the caller. So I think it could work to have:
1) all calls to virRun in veth.c request cmdResult.
2) all functions in veth.c log the errors themselves (with one possible
exception - vethDelete() - because it is often called during error
recovery).
3) all functions in veth.c return -1 to their caller
4) all functions called by those functions also return -1 (this is
mostly the case already, once you change veth.c, and has no effect on
those functions' callers).
Logging errors in veth.c functions you have all the information you need
from virRun, but also know the context in which the program is being
called (see my notes below) so you can avoid the "always big and scary"
messages from virRun, and you can consistently return -1.
(The only downside is that you don't have access to the stderr of the
executed program, but that will be solved when we switch to Dan
Berrange's new exec library ;-)
Does this sound reasonable and doable? If you agree with it, but think
it's not doable in time for the release on Friday, but the original bug
has real consequences (ie it makes something potentially fail), then I
think it would be okay to push your original patch, with intent to do a
further cleanup after the release.
(sorry for turning this 2 line bugfix into an odyssey of refactoring -
I'm just paying it forward ;-)
*****************************
Here are my notes from looking at the functions in veth.c and their callers:
vethCreate
called from:
lxcSetupInterfaces - on error, it logs "Failed to create device
pair %d", rc
solution - log that error in vethCreate, and have vethCreate return
-1 on failure
vethDelete
called from:
lxcControllerCleanupInterfaces - logs, but doesn't display code
lxcVmCleanup - ignores, no log
lxcVmStart - ignores, no log
Both of the cases where there is no log on failure are cases where there
was already an error, and this is just cleaning up. The other case is, I
think, as the container is shutting down. There's a good reason for not
logging an error if you're already in error recovery - the newer error
message will overwrite the old one (eg, virsh only prints out the last
error that was logged during any given API call).
suggestion - since the existing error log doesn't reference the
return code, just have vethDelete return -1, and continue logging the
error in the caller (when desired).
vethInterfaceUpOrDown
called from:
lxcContainerRenameAndEnableInterfaces - enabling, log error
lxcVmCleanup - disabling, don't log error
lxcSetupInterfaces - enabling, log error
This one doesn't log an error if upOrDown is 0, and in the two cases
where it does log an error, the text is nearly identical.
suggestion - log errors in vethInterfaceUpOrDown if upOrDown != 0,
and return -1 on failure. Don't log in the caller.
moveInterfaceToNetNs
called from: lxcControllerMoveInterfaces - error is logged, but
doesn't give code.
suggestion - do the logging in moveInterfaceToNetNs instead, and add
in the code, then return -1
setMacAddr
called from: lxcSetupInterfaces - error is logged, but doesn't give code
suggestion - do the logging in setMacAddr, and add in the code, then
return -1
setInterfaceName
called from: lxcContainerRenameAndEnableInterfaces - code is saved,
then used to log error
suggestion - log the same error in setInterfaceName that was
previously logged in the caller, then return -1.
5:22 p.m.
On Wed, Jul 28, 2010 at 10:15 AM, Laine Stump <laine(a)laine.org> wrote:
On 07/27/2010 11:11 AM, Ryota Ozaki wrote:
>
> On Mon, Jul 26, 2010 at 11:37 PM, Laine Stump<laine(a)laine.org> wrote:
>>
>> On 07/26/2010 08:31 AM, Ryota Ozaki wrote:
>>>
>>> On Mon, Jul 26, 2010 at 6:51 PM, Daniel P.
Berrange<berrange(a)redhat.com>
>>> wrote:
>>>>
>>>> You could just change
>>>>
>>>> return cmdResult
>>>>
>>>> to
>>>>
>>>> return -cmdResult;
>>>>
>>>> That would still let you give the error code, while also keeping the
>>>> value
>>>> < 0
>>>
>>> It looks better than mine ;-) I'll rewrite my patch in such a way.
>>>
>>> Laine, is it ok for you too?
>>>
>> Doing that is fine when all possible failures in the function have an
>> associated errno. In the case of virRun'ing an external program that
>> could
>> return a non-zero exit code, this is unfortunately not the case, so those
>> functions that call virRun will need to report the error themselves and
>> return -1.
>
> For veth.c all functions match the latter case while bridge.c has both.
> If we don't take care about the consistency between veth.c and bridge.c,
> we can focus on how to treat the latter case. (Ideally keeping the
> consistency
> is better, but it seems difficult...)
We can ignore bridge.c for now - there is a lot of code in libvirt that
doesn't follow the "-errno on failure" convention, so we should try to be
as
narrow as possible. For this cleanup, I like the scope of just veth.c and
its direct callers.
OK.
Another thing I didn't notice when I wrote my first comment on this thread
is that most of the error conditions in the involved functions are due to an
external program failing, which will likely be due to the program returning
a non-0 code. However, that's not *always* the case, so we can't really say
that the function will always return a valid "-exitcode", nor can we say it
will always return a valid "-errno" (and as you point out, they can
conflict).
I'm not sure the valid "-exitcode", do you mean that virRun itself may fail
and return non-zero (not cmdResult)?
>> When non-0 exits from the called program are all failures, the simplest
>> way
>> to do it is, as you say, to just not pass a pointer to a resultcode to
>> virRun (as long as the error message reported by virRun is useful enough
>> -
>
> Yes.
>
>> remember that it gives the name of the program being run, and
"virRun",
>> but
>> not the name of the calling function).
>
> Agreed. That'll lose useful information for debugging. One option is
> to re-report
> an error in the calling function. It will lead reporting two messages,
> but it should
> be more useful than less reports.
>
> One concern aobut virRun's error reporting is that it shows standard
> errors through
> DEBUG so by default it shows nothing while they are important for ifconfig
> and ip commands because their error messages may be according to errno.
I also recall once in some other code letting virRun display the error and
the result was that someone else who got the error (which was really
unimportant) was handed a giant scary looking (and uninformative) error
message that took their attention away from the real problem, which was
elsewhere (I forget the details now, but you get the idea). It's convenient,
but if you want meaningful errors, I would recommend against letting virRun
report them.
I agree virRun's error message isn't suit for displaying to users, but it's
for
debugging. What I want to say here is that I want to log the message
somewhere. (I'm sorry I didn't distinguish well between error reporting to
users and logging to a log file.)
BTW, lxc driver has a problem that lxc controller process cannot transfer
any errors to libvirtd (and to virsh). Errors which happen in the controller
may be lost. At this point, logging is the only way to store error messages.
(Am I wrong?)
(Even worse, libvirtd would return control back to virsh before a controller
surely executes init process of lxc. This defect leads a situation that virsh
exists successfully, but the container is actually not running. I thinks
we would have to fix those issues at some point.)
I went through all the calls to all the functions listed in veth.h, and I
see that they are called from relatively few places, and the error logs are
almost always to the veth.c function that's being called, not to the caller.
So I think it could work to have:
1) all calls to virRun in veth.c request cmdResult.
2) all functions in veth.c log the errors themselves (with one possible
exception - vethDelete() - because it is often called during error
recovery).
3) all functions in veth.c return -1 to their caller
4) all functions called by those functions also return -1 (this is mostly
the case already, once you change veth.c, and has no effect on those
functions' callers).
Logging errors in veth.c functions you have all the information you need
from virRun, but also know the context in which the program is being called
(see my notes below) so you can avoid the "always big and scary" messages
from virRun, and you can consistently return -1.
(The only downside is that you don't have access to the stderr of the
executed program, but that will be solved when we switch to Dan Berrange's
new exec library ;-)
Oh, I'm happy to hear it ;-) That's one of my concerns.
Does this sound reasonable and doable?
That sounds reasonable for me, though we need to review based on
real code and test.
Anyway, I've composed a patch which reflects your suggestions.
Please find the patch appended at the end of this mail.
If you agree with it, but think it's
not doable in time for the release on Friday, but the original bug has real
consequences (ie it makes something potentially fail), then I think it would
be okay to push your original patch, with intent to do a further cleanup
after the release.
The bug which my original patch fixes is not critical but it could cause
undesirable behavior; it allows a container to launch with the presence of
errors during startup. I guess that the bug is related to the bug report
https://bugzilla.redhat.com/show_bug.cgi?id=607496 .
So I'll resend my patch and let it be included if in time.
(sorry for turning this 2 line bugfix into an odyssey of refactoring - I'm
just paying it forward ;-)
No problem. Eventually we shall fix those inconsistencies at some point.
*****************************
Here are my notes from looking at the functions in veth.c and their callers:
Great! It's really helpful for me composing the bellow patch!
P.S. I'll be offline for a couple of days since Friday. That's a
reason why I was
a bit eager...
Thanks,
ozaki-r
vethCreate
called from:
lxcSetupInterfaces - on error, it logs "Failed to create device pair
%d", rc
solution - log that error in vethCreate, and have vethCreate return -1 on
failure
vethDelete
called from:
lxcControllerCleanupInterfaces - logs, but doesn't display code
lxcVmCleanup - ignores, no log
lxcVmStart - ignores, no log
Both of the cases where there is no log on failure are cases where there was
already an error, and this is just cleaning up. The other case is, I think,
as the container is shutting down. There's a good reason for not logging an
error if you're already in error recovery - the newer error message will
overwrite the old one (eg, virsh only prints out the last error that was
logged during any given API call).
suggestion - since the existing error log doesn't reference the return
code, just have vethDelete return -1, and continue logging the error in the
caller (when desired).
vethInterfaceUpOrDown
called from:
lxcContainerRenameAndEnableInterfaces - enabling, log error
lxcVmCleanup - disabling, don't log error
lxcSetupInterfaces - enabling, log error
This one doesn't log an error if upOrDown is 0, and in the two cases where
it does log an error, the text is nearly identical.
suggestion - log errors in vethInterfaceUpOrDown if upOrDown != 0, and
return -1 on failure. Don't log in the caller.
moveInterfaceToNetNs
called from: lxcControllerMoveInterfaces - error is logged, but doesn't
give code.
suggestion - do the logging in moveInterfaceToNetNs instead, and add in the
code, then return -1
setMacAddr
called from: lxcSetupInterfaces - error is logged, but doesn't give code
suggestion - do the logging in setMacAddr, and add in the code, then return
-1
setInterfaceName
called from: lxcContainerRenameAndEnableInterfaces - code is saved, then
used to log error
suggestion - log the same error in setInterfaceName that was previously
logged in the caller, then return -1.
diff --git a/po/POTFILES.in b/po/POTFILES.in
index dad1f8d..8a148f3 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -33,6 +33,7 @@ src/lxc/lxc_container.c
src/lxc/lxc_conf.c
src/lxc/lxc_controller.c
src/lxc/lxc_driver.c
+src/lxc/veth.c
src/network/bridge_driver.c
src/node_device/node_device_driver.c
src/node_device/node_device_hal.c
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..b19192f 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -255,20 +255,14 @@ static int
lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
DEBUG("Renaming %s to %s", veths[i], newname);
rc = setInterfaceName(veths[i], newname);
- if (0 != rc) {
- VIR_ERROR(_("Failed to rename %s to %s (%d)"),
- veths[i], newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
DEBUG("Enabling %s", newname);
rc = vethInterfaceUpOrDown(newname, 1);
- if (0 != rc) {
- VIR_ERROR(_("Failed to enable %s (%d)"), newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
+
VIR_FREE(newname);
}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d8b7bc7..7dc51a5 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -477,12 +477,8 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (moveInterfaceToNetNs(veths[i], container) < 0) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to move interface %s to ns %d"),
- veths[i], container);
+ if (moveInterfaceToNetNs(veths[i], container) < 0)
return -1;
- }
return 0;
}
@@ -502,10 +498,7 @@ static int
lxcControllerCleanupInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (vethDelete(veths[i]) < 0)
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to delete veth: %s"), veths[i]);
- /* will continue to try to cleanup any other interfaces */
+ vethDelete(veths[i]);
return 0;
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4fc1ecd..614548e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -859,11 +859,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
strcpy(parentVeth, def->nets[i]->ifname);
}
DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- if (0 != (rc = vethCreate(parentVeth, PATH_MAX,
containerVeth, PATH_MAX))) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to create veth device pair: %d"), rc);
+ rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX);
+ if (rc < 0)
goto error_exit;
- }
+
if (NULL == def->nets[i]->ifname) {
def->nets[i]->ifname = strdup(parentVeth);
}
@@ -885,12 +884,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
- if (0 != (rc = setMacAddr(containerVeth, macaddr))) {
- virReportSystemError(rc,
- _("Failed to set %s to %s"),
- macaddr, containerVeth);
+ rc = setMacAddr(containerVeth, macaddr);
+ if (rc < 0)
goto error_exit;
- }
}
if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
@@ -900,13 +896,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
goto error_exit;
}
- if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
- virReportSystemError(rc,
- _("Failed to enable %s device"),
- parentVeth);
+ rc = vethInterfaceUpOrDown(parentVeth, 1);
+ if (rc < 0)
goto error_exit;
- }
-
}
rc = 0;
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa..19f11f2 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -13,12 +13,21 @@
#include <string.h>
#include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
#include "veth.h"
#include "internal.h"
#include "logging.h"
#include "memory.h"
#include "util.h"
+#include "virterror_internal.h"
+
+#define VIR_FROM_THIS VIR_FROM_LXC
+
+#define vethError(code, ...) \
+ virReportErrorHelper(NULL, VIR_FROM_LXC, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
/* Functions */
/**
@@ -80,13 +89,9 @@ int vethCreate(char* veth1, int veth1MaxLen,
const char *argv[] = {
"ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
};
- int cmdResult;
+ int cmdResult = 0;
int vethDev = 0;
- if ((NULL == veth1) || (NULL == veth2)) {
- goto error_out;
- }
-
DEBUG("veth1: %s veth2: %s", veth1, veth2);
while ((1 > strlen(veth1)) || STREQ(veth1, veth2)) {
@@ -104,11 +109,14 @@ int vethCreate(char* veth1, int veth1MaxLen,
DEBUG("veth1: %s veth2: %s", veth1, veth2);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to create veth device pair '%s', '%s':
%d"),
+ veth1, veth2, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -127,21 +135,23 @@ int vethDelete(const char *veth)
{
int rc = -1;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
DEBUG("veth: %s", veth);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to delete '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -159,11 +169,7 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
{
int rc = -1;
const char *argv[] = {"ifconfig", veth, NULL, NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
if (0 == upOrDown)
argv[2] = "down";
@@ -172,11 +178,22 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ if (0 == upOrDown)
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to disable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ else
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to enable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -198,21 +215,23 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
const char *argv[] = {
"ip", "link", "set", iface, "netns",
NULL, NULL
};
- int cmdResult;
+ int cmdResult = 0;
- if (NULL == iface) {
- goto error_out;
+ if (virAsprintf(&pid, "%d", pidInNs) == -1) {
+ virReportOOMError();
+ return -1;
}
- if (virAsprintf(&pid, "%d", pidInNs) == -1)
- goto error_out;
-
argv[5] = pid;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to move '%s' into NS(pid=%d) (%d)"),
+ iface, pidInNs, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
VIR_FREE(pid);
return rc;
}
@@ -234,17 +253,17 @@ int setMacAddr(const char* iface, const char* macaddr)
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
- int cmdResult;
-
- if (NULL == iface) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ macaddr, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
@@ -265,16 +284,16 @@ int setInterfaceName(const char* iface, const char* new)
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
- int cmdResult;
-
- if (NULL == iface || NULL == new) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ new, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
diff --git a/src/lxc/veth.h b/src/lxc/veth.h
index 523bf9c..1ec1ec8 100644
--- a/src/lxc/veth.h
+++ b/src/lxc/veth.h
@@ -13,14 +13,21 @@
# define VETH_H
# include <config.h>
+# include "internal.h"
/* Function declarations */
int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
- int veth2MaxLen);
-int vethDelete(const char* veth);
-int vethInterfaceUpOrDown(const char* veth, int upOrDown);
-int moveInterfaceToNetNs(const char *iface, int pidInNs);
-int setMacAddr(const char* iface, const char* macaddr);
-int setInterfaceName(const char* iface, const char* new);
+ int veth2MaxLen)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+int vethDelete(const char* veth)
+ ATTRIBUTE_NONNULL(1);
+int vethInterfaceUpOrDown(const char* veth, int upOrDown)
+ ATTRIBUTE_NONNULL(1);
+int moveInterfaceToNetNs(const char *iface, int pidInNs)
+ ATTRIBUTE_NONNULL(1);
+int setMacAddr(const char* iface, const char* macaddr)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int setInterfaceName(const char* iface, const char* new)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
#endif /* VETH_H */
11:18 a.m.
New subject: [libvirt] [PATCH revision RFC 0/2] lxc: Fix return values of veth.c functions
The first of thse two patches is an uncorrupted version of the patch in the parent
message. The second is a few suggested changes. Overall, it looks
very good.
If you're okay with these changes, and at least one other person ACKs
the combination, I can squash the two patches together and push the result.
11:18 a.m.
New subject: [libvirt] [PATCH revision RFC 1/2] lxc: Fix return values of veth.c functions
From: Ryota Ozaki <ozaki.ryota(a)gmail.com>
Previously, the functions in src/lxc/veth.c could sometimes return
positive values on failure rather than -1. This made accurate error
reporting difficult, and led to one failure to catch an error in a
calling function.
This patch makes all the functions in veth.c consistently return 0 on
success, and -1 on failure. It also fixes up the callers to the veth.c
functions where necessary.
Note that this patch may be related to the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=607496.
It would not fix the bug, but would unveil what happens.
* po/POTFILES.in - add veth.c, which previously had no translatable strings
* src/lxc/lxc_controller.c
* src/lxc/lxc_container.c
* src/lxc/lxc_driver.c - fixup callers to veth.c, and remove error logs,
as they are now done in veth.c
* src/lxc/veth.c - make all functions consistently return -1 on error.
* src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args.
---
po/POTFILES.in | 1 +
src/lxc/lxc_container.c | 12 +----
src/lxc/lxc_controller.c | 11 +----
src/lxc/lxc_driver.c | 22 +++------
src/lxc/veth.c | 117 ++++++++++++++++++++++++++-------------------
src/lxc/veth.h | 19 +++++--
6 files changed, 94 insertions(+), 88 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index dad1f8d..8a148f3 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -33,6 +33,7 @@ src/lxc/lxc_container.c
src/lxc/lxc_conf.c
src/lxc/lxc_controller.c
src/lxc/lxc_driver.c
+src/lxc/veth.c
src/network/bridge_driver.c
src/node_device/node_device_driver.c
src/node_device/node_device_hal.c
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..b19192f 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -255,20 +255,14 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int
nveths,
DEBUG("Renaming %s to %s", veths[i], newname);
rc = setInterfaceName(veths[i], newname);
- if (0 != rc) {
- VIR_ERROR(_("Failed to rename %s to %s (%d)"),
- veths[i], newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
DEBUG("Enabling %s", newname);
rc = vethInterfaceUpOrDown(newname, 1);
- if (0 != rc) {
- VIR_ERROR(_("Failed to enable %s (%d)"), newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
+
VIR_FREE(newname);
}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d8b7bc7..7dc51a5 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -477,12 +477,8 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (moveInterfaceToNetNs(veths[i], container) < 0) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to move interface %s to ns %d"),
- veths[i], container);
+ if (moveInterfaceToNetNs(veths[i], container) < 0)
return -1;
- }
return 0;
}
@@ -502,10 +498,7 @@ static int lxcControllerCleanupInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (vethDelete(veths[i]) < 0)
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to delete veth: %s"), veths[i]);
- /* will continue to try to cleanup any other interfaces */
+ vethDelete(veths[i]);
return 0;
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4fc1ecd..614548e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -859,11 +859,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
strcpy(parentVeth, def->nets[i]->ifname);
}
DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- if (0 != (rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX))) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to create veth device pair: %d"), rc);
+ rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX);
+ if (rc < 0)
goto error_exit;
- }
+
if (NULL == def->nets[i]->ifname) {
def->nets[i]->ifname = strdup(parentVeth);
}
@@ -885,12 +884,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
- if (0 != (rc = setMacAddr(containerVeth, macaddr))) {
- virReportSystemError(rc,
- _("Failed to set %s to %s"),
- macaddr, containerVeth);
+ rc = setMacAddr(containerVeth, macaddr);
+ if (rc < 0)
goto error_exit;
- }
}
if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
@@ -900,13 +896,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
goto error_exit;
}
- if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
- virReportSystemError(rc,
- _("Failed to enable %s device"),
- parentVeth);
+ rc = vethInterfaceUpOrDown(parentVeth, 1);
+ if (rc < 0)
goto error_exit;
- }
-
}
rc = 0;
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa..19f11f2 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -13,12 +13,21 @@
#include <string.h>
#include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
#include "veth.h"
#include "internal.h"
#include "logging.h"
#include "memory.h"
#include "util.h"
+#include "virterror_internal.h"
+
+#define VIR_FROM_THIS VIR_FROM_LXC
+
+#define vethError(code, ...) \
+ virReportErrorHelper(NULL, VIR_FROM_LXC, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
/* Functions */
/**
@@ -80,13 +89,9 @@ int vethCreate(char* veth1, int veth1MaxLen,
const char *argv[] = {
"ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
};
- int cmdResult;
+ int cmdResult = 0;
int vethDev = 0;
- if ((NULL == veth1) || (NULL == veth2)) {
- goto error_out;
- }
-
DEBUG("veth1: %s veth2: %s", veth1, veth2);
while ((1 > strlen(veth1)) || STREQ(veth1, veth2)) {
@@ -104,11 +109,14 @@ int vethCreate(char* veth1, int veth1MaxLen,
DEBUG("veth1: %s veth2: %s", veth1, veth2);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to create veth device pair '%s', '%s':
%d"),
+ veth1, veth2, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -127,21 +135,23 @@ int vethDelete(const char *veth)
{
int rc = -1;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
DEBUG("veth: %s", veth);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to delete '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -159,11 +169,7 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
{
int rc = -1;
const char *argv[] = {"ifconfig", veth, NULL, NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
if (0 == upOrDown)
argv[2] = "down";
@@ -172,11 +178,22 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ if (0 == upOrDown)
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to disable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ else
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to enable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -198,21 +215,23 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
const char *argv[] = {
"ip", "link", "set", iface, "netns",
NULL, NULL
};
- int cmdResult;
+ int cmdResult = 0;
- if (NULL == iface) {
- goto error_out;
+ if (virAsprintf(&pid, "%d", pidInNs) == -1) {
+ virReportOOMError();
+ return -1;
}
- if (virAsprintf(&pid, "%d", pidInNs) == -1)
- goto error_out;
-
argv[5] = pid;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to move '%s' into NS(pid=%d) (%d)"),
+ iface, pidInNs, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
VIR_FREE(pid);
return rc;
}
@@ -234,17 +253,17 @@ int setMacAddr(const char* iface, const char* macaddr)
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
- int cmdResult;
-
- if (NULL == iface) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ macaddr, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
@@ -265,16 +284,16 @@ int setInterfaceName(const char* iface, const char* new)
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
- int cmdResult;
-
- if (NULL == iface || NULL == new) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ new, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
diff --git a/src/lxc/veth.h b/src/lxc/veth.h
index 523bf9c..1ec1ec8 100644
--- a/src/lxc/veth.h
+++ b/src/lxc/veth.h
@@ -13,14 +13,21 @@
# define VETH_H
# include <config.h>
+# include "internal.h"
/* Function declarations */
int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
- int veth2MaxLen);
-int vethDelete(const char* veth);
-int vethInterfaceUpOrDown(const char* veth, int upOrDown);
-int moveInterfaceToNetNs(const char *iface, int pidInNs);
-int setMacAddr(const char* iface, const char* macaddr);
-int setInterfaceName(const char* iface, const char* new);
+ int veth2MaxLen)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+int vethDelete(const char* veth)
+ ATTRIBUTE_NONNULL(1);
+int vethInterfaceUpOrDown(const char* veth, int upOrDown)
+ ATTRIBUTE_NONNULL(1);
+int moveInterfaceToNetNs(const char *iface, int pidInNs)
+ ATTRIBUTE_NONNULL(1);
+int setMacAddr(const char* iface, const char* macaddr)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int setInterfaceName(const char* iface, const char* new)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
#endif /* VETH_H */
--
1.7.1.1
12:24 p.m.
New subject: [libvirt] [PATCH revision RFC 1/2] lxc: Fix return values of veth.c functions
On Thu, Jul 29, 2010 at 12:18:23PM -0400, Laine Stump wrote:
From: Ryota Ozaki <ozaki.ryota(a)gmail.com>
Previously, the functions in src/lxc/veth.c could sometimes return
positive values on failure rather than -1. This made accurate error
reporting difficult, and led to one failure to catch an error in a
calling function.
This patch makes all the functions in veth.c consistently return 0 on
success, and -1 on failure. It also fixes up the callers to the veth.c
functions where necessary.
Note that this patch may be related to the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=607496.
It would not fix the bug, but would unveil what happens.
* po/POTFILES.in - add veth.c, which previously had no translatable strings
* src/lxc/lxc_controller.c
* src/lxc/lxc_container.c
* src/lxc/lxc_driver.c - fixup callers to veth.c, and remove error logs,
as they are now done in veth.c
* src/lxc/veth.c - make all functions consistently return -1 on error.
* src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args.
---
po/POTFILES.in | 1 +
src/lxc/lxc_container.c | 12 +----
src/lxc/lxc_controller.c | 11 +----
src/lxc/lxc_driver.c | 22 +++------
src/lxc/veth.c | 117 ++++++++++++++++++++++++++-------------------
src/lxc/veth.h | 19 +++++--
6 files changed, 94 insertions(+), 88 deletions(-)
ACK looks fine to me,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
11:18 a.m.
New subject: [libvirt] [PATCH revision RFC 2/2] lxc: Fix return values of veth.c functions - suggested changes
Some suggested changes to your latest patch (I did the review by
applying your patch, then examining the functions that were touched,
focusing just on setting of rc)
Summary:
1) virAsprintf() will return the number of characters in the new
string on success, not 0, so we need to only set rc if it fails
(< 0). Assigning rc on success causes the caller to falsely believe
the function failed.
2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even
if waitpid had failed. I don't know if the behavior of WIFEXITED
is defined if waitpid fails, but all the other uses I can find
avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's
what I did here.
3) lxcSetupInterfaces - rather than explicitly setting rc from the
return of functions, since it defaults to -1, I just goto
error_exit if the functions return < 0. That's just cosmetic. The
real problem is that rc was being set from brAddInterface, which
returns > 0 on failure.
4) assigning "rc = -1" at the beginning of each veth.c function is a
dead store, since it will always be set by the call to virRun(). This
causes coverity code analysis tool to report problems.
---
src/lxc/lxc_container.c | 6 ++++--
src/lxc/lxc_driver.c | 19 ++++++-------------
src/lxc/veth.c | 12 ++++++------
3 files changed, 16 insertions(+), 21 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index b19192f..b7f025a 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -249,9 +249,11 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int
nveths,
char *newname = NULL;
for (i = 0 ; i < nveths ; i++) {
- rc = virAsprintf(&newname, "eth%d", i);
- if (rc < 0)
+ if (virAsprintf(&newname, "eth%d", i) < 0) {
+ virReportOOMError();
+ rc = -1;
goto error_out;
+ }
DEBUG("Renaming %s to %s", veths[i], newname);
rc = setInterfaceName(veths[i], newname);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 614548e..9238f80 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -722,7 +722,7 @@ cleanup:
static int lxcVmCleanup(lxc_driver_t *driver,
virDomainObjPtr vm)
{
- int rc = -1;
+ int rc = 0;
int waitRc;
int childStatus = -1;
virCgroupPtr cgroup;
@@ -737,11 +737,7 @@ static int lxcVmCleanup(lxc_driver_t *driver,
virReportSystemError(errno,
_("waitpid failed to wait for container %d: %d"),
vm->pid, waitRc);
- }
-
- rc = 0;
-
- if (WIFEXITED(childStatus)) {
+ } else if (WIFEXITED(childStatus)) {
rc = WEXITSTATUS(childStatus);
DEBUG("container exited with rc: %d", rc);
}
@@ -859,8 +855,7 @@ static int lxcSetupInterfaces(virConnectPtr conn,
strcpy(parentVeth, def->nets[i]->ifname);
}
DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX);
- if (rc < 0)
+ if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0)
goto error_exit;
if (NULL == def->nets[i]->ifname) {
@@ -884,20 +879,18 @@ static int lxcSetupInterfaces(virConnectPtr conn,
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
- rc = setMacAddr(containerVeth, macaddr);
- if (rc < 0)
+ if (setMacAddr(containerVeth, macaddr) < 0)
goto error_exit;
}
- if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
+ if (0 != brAddInterface(brctl, bridge, parentVeth)) {
virReportSystemError(rc,
_("Failed to add %s device to %s"),
parentVeth, bridge);
goto error_exit;
}
- rc = vethInterfaceUpOrDown(parentVeth, 1);
- if (rc < 0)
+ if (vethInterfaceUpOrDown(parentVeth, 1) < 0)
goto error_exit;
}
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 19f11f2..acf28c7 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -85,7 +85,7 @@ static int getFreeVethName(char *veth, int maxLen, int startDev)
int vethCreate(char* veth1, int veth1MaxLen,
char* veth2, int veth2MaxLen)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
};
@@ -133,7 +133,7 @@ int vethCreate(char* veth1, int veth1MaxLen,
*/
int vethDelete(const char *veth)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
int cmdResult = 0;
@@ -167,7 +167,7 @@ int vethDelete(const char *veth)
*/
int vethInterfaceUpOrDown(const char* veth, int upOrDown)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ifconfig", veth, NULL, NULL};
int cmdResult = 0;
@@ -210,7 +210,7 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
*/
int moveInterfaceToNetNs(const char* iface, int pidInNs)
{
- int rc = -1;
+ int rc;
char *pid = NULL;
const char *argv[] = {
"ip", "link", "set", iface, "netns",
NULL, NULL
@@ -249,7 +249,7 @@ int moveInterfaceToNetNs(const char* iface, int pidInNs)
*/
int setMacAddr(const char* iface, const char* macaddr)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
@@ -280,7 +280,7 @@ int setMacAddr(const char* iface, const char* macaddr)
*/
int setInterfaceName(const char* iface, const char* new)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
--
1.7.1.1
12:29 p.m.
New subject: [libvirt] [PATCH revision RFC 2/2] lxc: Fix return values of veth.c functions - suggested changes
On Thu, Jul 29, 2010 at 12:18:24PM -0400, Laine Stump wrote:
Some suggested changes to your latest patch (I did the review by
applying your patch, then examining the functions that were touched,
focusing just on setting of rc)
Summary:
1) virAsprintf() will return the number of characters in the new
string on success, not 0, so we need to only set rc if it fails
(< 0). Assigning rc on success causes the caller to falsely believe
the function failed.
2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even
if waitpid had failed. I don't know if the behavior of WIFEXITED
is defined if waitpid fails, but all the other uses I can find
avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's
what I did here.
3) lxcSetupInterfaces - rather than explicitly setting rc from the
return of functions, since it defaults to -1, I just goto
error_exit if the functions return < 0. That's just cosmetic. The
real problem is that rc was being set from brAddInterface, which
returns > 0 on failure.
4) assigning "rc = -1" at the beginning of each veth.c function is a
dead store, since it will always be set by the call to virRun(). This
causes coverity code analysis tool to report problems.
---
src/lxc/lxc_container.c | 6 ++++--
src/lxc/lxc_driver.c | 19 ++++++-------------
src/lxc/veth.c | 12 ++++++------
3 files changed, 16 insertions(+), 21 deletions(-)
Okay, looks fine too, ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
12:32 p.m.
New subject: [libvirt] [PATCHv3] lxc: Fix return values of veth.c functions - complete
From: Ryota Ozaki <ozaki.ryota(a)gmail.com>
From: Ryota Ozaki <ozaki.ryota(a)gmail.com>
Previously, the functions in src/lxc/veth.c could sometimes return
positive values on failure rather than -1. This made accurate error
reporting difficult, and led to one failure to catch an error in a
calling function.
This patch makes all the functions in veth.c consistently return 0 on
success, and -1 on failure. It also fixes up the callers to the veth.c
functions where necessary.
Note that this patch may be related to the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=607496.
It will not fix the bug, but should unveil what happens.
* po/POTFILES.in - add veth.c, which previously had no translatable strings
* src/lxc/lxc_controller.c
* src/lxc/lxc_container.c
* src/lxc/lxc_driver.c - fixup callers to veth.c, and remove error logs,
as they are now done in veth.c
* src/lxc/veth.c - make all functions consistently return -1 on error.
* src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args.
---
Changes from Ozaki's original patch:
1) virAsprintf() will return the number of characters in the new
string on success, not 0, so we need to only set rc if it fails
(< 0). Assigning rc on success causes the caller to falsely believe
the function failed.
2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even
if waitpid had failed. I don't know if the behavior of WIFEXITED
is defined if waitpid fails, but all the other uses I can find
avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's
what I did here. Also changed to return -1 if waitpid fails or
WEXITSTATUS != 0.
3) lxcSetupInterfaces - rather than explicitly setting rc from the
return of functions, since it defaults to -1, I just goto
error_exit if the functions return < 0. That's just cosmetic. The
real problem is that rc was being set from brAddInterface, which
returns > 0 on failure (note error log of errno from brAddInterface
fixed in v3).
4) assigning "rc = -1" at the beginning of each veth.c function is a
dead store, since it will always be set by the call to virRun(). This
causes coverity code analysis tool to report problems.
po/POTFILES.in | 1 +
src/lxc/lxc_container.c | 18 +++----
src/lxc/lxc_controller.c | 11 +---
src/lxc/lxc_driver.c | 33 ++++--------
src/lxc/veth.c | 129 ++++++++++++++++++++++++++-------------------
src/lxc/veth.h | 19 +++++--
6 files changed, 107 insertions(+), 104 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index dad1f8d..8a148f3 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -33,6 +33,7 @@ src/lxc/lxc_container.c
src/lxc/lxc_conf.c
src/lxc/lxc_controller.c
src/lxc/lxc_driver.c
+src/lxc/veth.c
src/network/bridge_driver.c
src/node_device/node_device_driver.c
src/node_device/node_device_hal.c
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..b7f025a 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -249,26 +249,22 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int
nveths,
char *newname = NULL;
for (i = 0 ; i < nveths ; i++) {
- rc = virAsprintf(&newname, "eth%d", i);
- if (rc < 0)
+ if (virAsprintf(&newname, "eth%d", i) < 0) {
+ virReportOOMError();
+ rc = -1;
goto error_out;
+ }
DEBUG("Renaming %s to %s", veths[i], newname);
rc = setInterfaceName(veths[i], newname);
- if (0 != rc) {
- VIR_ERROR(_("Failed to rename %s to %s (%d)"),
- veths[i], newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
DEBUG("Enabling %s", newname);
rc = vethInterfaceUpOrDown(newname, 1);
- if (0 != rc) {
- VIR_ERROR(_("Failed to enable %s (%d)"), newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
+
VIR_FREE(newname);
}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d8b7bc7..7dc51a5 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -477,12 +477,8 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (moveInterfaceToNetNs(veths[i], container) < 0) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to move interface %s to ns %d"),
- veths[i], container);
+ if (moveInterfaceToNetNs(veths[i], container) < 0)
return -1;
- }
return 0;
}
@@ -502,10 +498,7 @@ static int lxcControllerCleanupInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (vethDelete(veths[i]) < 0)
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to delete veth: %s"), veths[i]);
- /* will continue to try to cleanup any other interfaces */
+ vethDelete(veths[i]);
return 0;
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4fc1ecd..bc48b59 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -722,7 +722,7 @@ cleanup:
static int lxcVmCleanup(lxc_driver_t *driver,
virDomainObjPtr vm)
{
- int rc = -1;
+ int rc = 0;
int waitRc;
int childStatus = -1;
virCgroupPtr cgroup;
@@ -737,13 +737,10 @@ static int lxcVmCleanup(lxc_driver_t *driver,
virReportSystemError(errno,
_("waitpid failed to wait for container %d: %d"),
vm->pid, waitRc);
- }
-
- rc = 0;
-
- if (WIFEXITED(childStatus)) {
- rc = WEXITSTATUS(childStatus);
- DEBUG("container exited with rc: %d", rc);
+ rc = -1;
+ } else if (WIFEXITED(childStatus)) {
+ DEBUG("container exited with rc: %d", WEXITSTATUS(childStatus));
+ rc = -1;
}
/* now that we know it's stopped call the hook if present */
@@ -859,11 +856,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
strcpy(parentVeth, def->nets[i]->ifname);
}
DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- if (0 != (rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX))) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to create veth device pair: %d"), rc);
+ if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0)
goto error_exit;
- }
+
if (NULL == def->nets[i]->ifname) {
def->nets[i]->ifname = strdup(parentVeth);
}
@@ -885,28 +880,20 @@ static int lxcSetupInterfaces(virConnectPtr conn,
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
- if (0 != (rc = setMacAddr(containerVeth, macaddr))) {
- virReportSystemError(rc,
- _("Failed to set %s to %s"),
- macaddr, containerVeth);
+ if (setMacAddr(containerVeth, macaddr) < 0)
goto error_exit;
- }
}
if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
virReportSystemError(rc,
_("Failed to add %s device to %s"),
parentVeth, bridge);
+ rc = -1;
goto error_exit;
}
- if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
- virReportSystemError(rc,
- _("Failed to enable %s device"),
- parentVeth);
+ if (vethInterfaceUpOrDown(parentVeth, 1) < 0)
goto error_exit;
- }
-
}
rc = 0;
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa..acf28c7 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -13,12 +13,21 @@
#include <string.h>
#include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
#include "veth.h"
#include "internal.h"
#include "logging.h"
#include "memory.h"
#include "util.h"
+#include "virterror_internal.h"
+
+#define VIR_FROM_THIS VIR_FROM_LXC
+
+#define vethError(code, ...) \
+ virReportErrorHelper(NULL, VIR_FROM_LXC, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
/* Functions */
/**
@@ -76,17 +85,13 @@ static int getFreeVethName(char *veth, int maxLen, int startDev)
int vethCreate(char* veth1, int veth1MaxLen,
char* veth2, int veth2MaxLen)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
};
- int cmdResult;
+ int cmdResult = 0;
int vethDev = 0;
- if ((NULL == veth1) || (NULL == veth2)) {
- goto error_out;
- }
-
DEBUG("veth1: %s veth2: %s", veth1, veth2);
while ((1 > strlen(veth1)) || STREQ(veth1, veth2)) {
@@ -104,11 +109,14 @@ int vethCreate(char* veth1, int veth1MaxLen,
DEBUG("veth1: %s veth2: %s", veth1, veth2);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to create veth device pair '%s', '%s':
%d"),
+ veth1, veth2, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -125,23 +133,25 @@ error_out:
*/
int vethDelete(const char *veth)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
DEBUG("veth: %s", veth);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to delete '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -157,13 +167,9 @@ error_out:
*/
int vethInterfaceUpOrDown(const char* veth, int upOrDown)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ifconfig", veth, NULL, NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
if (0 == upOrDown)
argv[2] = "down";
@@ -172,11 +178,22 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ if (0 == upOrDown)
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to disable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ else
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to enable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -193,26 +210,28 @@ error_out:
*/
int moveInterfaceToNetNs(const char* iface, int pidInNs)
{
- int rc = -1;
+ int rc;
char *pid = NULL;
const char *argv[] = {
"ip", "link", "set", iface, "netns",
NULL, NULL
};
- int cmdResult;
+ int cmdResult = 0;
- if (NULL == iface) {
- goto error_out;
+ if (virAsprintf(&pid, "%d", pidInNs) == -1) {
+ virReportOOMError();
+ return -1;
}
- if (virAsprintf(&pid, "%d", pidInNs) == -1)
- goto error_out;
-
argv[5] = pid;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to move '%s' into NS(pid=%d) (%d)"),
+ iface, pidInNs, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
VIR_FREE(pid);
return rc;
}
@@ -230,21 +249,21 @@ error_out:
*/
int setMacAddr(const char* iface, const char* macaddr)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
- int cmdResult;
-
- if (NULL == iface) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ macaddr, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
@@ -261,20 +280,20 @@ error_out:
*/
int setInterfaceName(const char* iface, const char* new)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
- int cmdResult;
-
- if (NULL == iface || NULL == new) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ new, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
diff --git a/src/lxc/veth.h b/src/lxc/veth.h
index 523bf9c..1ec1ec8 100644
--- a/src/lxc/veth.h
+++ b/src/lxc/veth.h
@@ -13,14 +13,21 @@
# define VETH_H
# include <config.h>
+# include "internal.h"
/* Function declarations */
int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
- int veth2MaxLen);
-int vethDelete(const char* veth);
-int vethInterfaceUpOrDown(const char* veth, int upOrDown);
-int moveInterfaceToNetNs(const char *iface, int pidInNs);
-int setMacAddr(const char* iface, const char* macaddr);
-int setInterfaceName(const char* iface, const char* new);
+ int veth2MaxLen)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+int vethDelete(const char* veth)
+ ATTRIBUTE_NONNULL(1);
+int vethInterfaceUpOrDown(const char* veth, int upOrDown)
+ ATTRIBUTE_NONNULL(1);
+int moveInterfaceToNetNs(const char *iface, int pidInNs)
+ ATTRIBUTE_NONNULL(1);
+int setMacAddr(const char* iface, const char* macaddr)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int setInterfaceName(const char* iface, const char* new)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
#endif /* VETH_H */
--
1.7.1.1
12:53 p.m.
New subject: [libvirt] [PATCHv3] lxc: Fix return values of veth.c functions - complete
On Fri, Jul 30, 2010 at 2:32 AM, Laine Stump <laine(a)laine.org> wrote:
From: Ryota Ozaki <ozaki.ryota(a)gmail.com>
From: Ryota Ozaki <ozaki.ryota(a)gmail.com>
Previously, the functions in src/lxc/veth.c could sometimes return
positive values on failure rather than -1. This made accurate error
reporting difficult, and led to one failure to catch an error in a
calling function.
This patch makes all the functions in veth.c consistently return 0 on
success, and -1 on failure. It also fixes up the callers to the veth.c
functions where necessary.
Note that this patch may be related to the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=607496.
It will not fix the bug, but should unveil what happens.
* po/POTFILES.in - add veth.c, which previously had no translatable strings
* src/lxc/lxc_controller.c
* src/lxc/lxc_container.c
* src/lxc/lxc_driver.c - fixup callers to veth.c, and remove error logs,
as they are now done in veth.c
* src/lxc/veth.c - make all functions consistently return -1 on error.
* src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args.
---
Changes from Ozaki's original patch:
1) virAsprintf() will return the number of characters in the new
string on success, not 0, so we need to only set rc if it fails
(< 0). Assigning rc on success causes the caller to falsely believe
the function failed.
2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even
if waitpid had failed. I don't know if the behavior of WIFEXITED
is defined if waitpid fails, but all the other uses I can find
avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's
what I did here. Also changed to return -1 if waitpid fails or
WEXITSTATUS != 0.
3) lxcSetupInterfaces - rather than explicitly setting rc from the
return of functions, since it defaults to -1, I just goto
error_exit if the functions return < 0. That's just cosmetic. The
real problem is that rc was being set from brAddInterface, which
returns > 0 on failure (note error log of errno from brAddInterface
fixed in v3).
4) assigning "rc = -1" at the beginning of each veth.c function is a
dead store, since it will always be set by the call to virRun(). This
causes coverity code analysis tool to report problems.
ACK. Thanks, Laine!
ozaki-r
po/POTFILES.in | 1 +
src/lxc/lxc_container.c | 18 +++----
src/lxc/lxc_controller.c | 11 +---
src/lxc/lxc_driver.c | 33 ++++--------
src/lxc/veth.c | 129 ++++++++++++++++++++++++++-------------------
src/lxc/veth.h | 19 +++++--
6 files changed, 107 insertions(+), 104 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index dad1f8d..8a148f3 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -33,6 +33,7 @@ src/lxc/lxc_container.c
src/lxc/lxc_conf.c
src/lxc/lxc_controller.c
src/lxc/lxc_driver.c
+src/lxc/veth.c
src/network/bridge_driver.c
src/node_device/node_device_driver.c
src/node_device/node_device_hal.c
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 4371dba..b7f025a 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -249,26 +249,22 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int
nveths,
char *newname = NULL;
for (i = 0 ; i < nveths ; i++) {
- rc = virAsprintf(&newname, "eth%d", i);
- if (rc < 0)
+ if (virAsprintf(&newname, "eth%d", i) < 0) {
+ virReportOOMError();
+ rc = -1;
goto error_out;
+ }
DEBUG("Renaming %s to %s", veths[i], newname);
rc = setInterfaceName(veths[i], newname);
- if (0 != rc) {
- VIR_ERROR(_("Failed to rename %s to %s (%d)"),
- veths[i], newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
DEBUG("Enabling %s", newname);
rc = vethInterfaceUpOrDown(newname, 1);
- if (0 != rc) {
- VIR_ERROR(_("Failed to enable %s (%d)"), newname, rc);
- rc = -1;
+ if (rc < 0)
goto error_out;
- }
+
VIR_FREE(newname);
}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index d8b7bc7..7dc51a5 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -477,12 +477,8 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (moveInterfaceToNetNs(veths[i], container) < 0) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to move interface %s to ns %d"),
- veths[i], container);
+ if (moveInterfaceToNetNs(veths[i], container) < 0)
return -1;
- }
return 0;
}
@@ -502,10 +498,7 @@ static int lxcControllerCleanupInterfaces(unsigned int nveths,
{
unsigned int i;
for (i = 0 ; i < nveths ; i++)
- if (vethDelete(veths[i]) < 0)
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to delete veth: %s"), veths[i]);
- /* will continue to try to cleanup any other interfaces */
+ vethDelete(veths[i]);
return 0;
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4fc1ecd..bc48b59 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -722,7 +722,7 @@ cleanup:
static int lxcVmCleanup(lxc_driver_t *driver,
virDomainObjPtr vm)
{
- int rc = -1;
+ int rc = 0;
int waitRc;
int childStatus = -1;
virCgroupPtr cgroup;
@@ -737,13 +737,10 @@ static int lxcVmCleanup(lxc_driver_t *driver,
virReportSystemError(errno,
_("waitpid failed to wait for container %d: %d"),
vm->pid, waitRc);
- }
-
- rc = 0;
-
- if (WIFEXITED(childStatus)) {
- rc = WEXITSTATUS(childStatus);
- DEBUG("container exited with rc: %d", rc);
+ rc = -1;
+ } else if (WIFEXITED(childStatus)) {
+ DEBUG("container exited with rc: %d", WEXITSTATUS(childStatus));
+ rc = -1;
}
/* now that we know it's stopped call the hook if present */
@@ -859,11 +856,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
strcpy(parentVeth, def->nets[i]->ifname);
}
DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- if (0 != (rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX))) {
- lxcError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to create veth device pair: %d"), rc);
+ if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0)
goto error_exit;
- }
+
if (NULL == def->nets[i]->ifname) {
def->nets[i]->ifname = strdup(parentVeth);
}
@@ -885,28 +880,20 @@ static int lxcSetupInterfaces(virConnectPtr conn,
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
- if (0 != (rc = setMacAddr(containerVeth, macaddr))) {
- virReportSystemError(rc,
- _("Failed to set %s to %s"),
- macaddr, containerVeth);
+ if (setMacAddr(containerVeth, macaddr) < 0)
goto error_exit;
- }
}
if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
virReportSystemError(rc,
_("Failed to add %s device to %s"),
parentVeth, bridge);
+ rc = -1;
goto error_exit;
}
- if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
- virReportSystemError(rc,
- _("Failed to enable %s device"),
- parentVeth);
+ if (vethInterfaceUpOrDown(parentVeth, 1) < 0)
goto error_exit;
- }
-
}
rc = 0;
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 34f7faa..acf28c7 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -13,12 +13,21 @@
#include <string.h>
#include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
#include "veth.h"
#include "internal.h"
#include "logging.h"
#include "memory.h"
#include "util.h"
+#include "virterror_internal.h"
+
+#define VIR_FROM_THIS VIR_FROM_LXC
+
+#define vethError(code, ...) \
+ virReportErrorHelper(NULL, VIR_FROM_LXC, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
/* Functions */
/**
@@ -76,17 +85,13 @@ static int getFreeVethName(char *veth, int maxLen, int startDev)
int vethCreate(char* veth1, int veth1MaxLen,
char* veth2, int veth2MaxLen)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
};
- int cmdResult;
+ int cmdResult = 0;
int vethDev = 0;
- if ((NULL == veth1) || (NULL == veth2)) {
- goto error_out;
- }
-
DEBUG("veth1: %s veth2: %s", veth1, veth2);
while ((1 > strlen(veth1)) || STREQ(veth1, veth2)) {
@@ -104,11 +109,14 @@ int vethCreate(char* veth1, int veth1MaxLen,
DEBUG("veth1: %s veth2: %s", veth1, veth2);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to create veth device pair '%s', '%s':
%d"),
+ veth1, veth2, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -125,23 +133,25 @@ error_out:
*/
int vethDelete(const char *veth)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ip", "link", "del", veth,
NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
DEBUG("veth: %s", veth);
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to delete '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -157,13 +167,9 @@ error_out:
*/
int vethInterfaceUpOrDown(const char* veth, int upOrDown)
{
- int rc = -1;
+ int rc;
const char *argv[] = {"ifconfig", veth, NULL, NULL};
- int cmdResult;
-
- if (NULL == veth) {
- goto error_out;
- }
+ int cmdResult = 0;
if (0 == upOrDown)
argv[2] = "down";
@@ -172,11 +178,22 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown)
rc = virRun(argv, &cmdResult);
- if (0 == rc) {
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ if (0 == upOrDown)
+ /*
+ * Prevent overwriting an error log which may be set
+ * where an actual failure occurs.
+ */
+ VIR_DEBUG(_("Failed to disable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ else
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to enable '%s' (%d)"),
+ veth, WEXITSTATUS(cmdResult));
+ rc = -1;
}
-error_out:
return rc;
}
@@ -193,26 +210,28 @@ error_out:
*/
int moveInterfaceToNetNs(const char* iface, int pidInNs)
{
- int rc = -1;
+ int rc;
char *pid = NULL;
const char *argv[] = {
"ip", "link", "set", iface, "netns",
NULL, NULL
};
- int cmdResult;
+ int cmdResult = 0;
- if (NULL == iface) {
- goto error_out;
+ if (virAsprintf(&pid, "%d", pidInNs) == -1) {
+ virReportOOMError();
+ return -1;
}
- if (virAsprintf(&pid, "%d", pidInNs) == -1)
- goto error_out;
-
argv[5] = pid;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to move '%s' into NS(pid=%d) (%d)"),
+ iface, pidInNs, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
VIR_FREE(pid);
return rc;
}
@@ -230,21 +249,21 @@ error_out:
*/
int setMacAddr(const char* iface, const char* macaddr)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "address",
macaddr, NULL
};
- int cmdResult;
-
- if (NULL == iface) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ macaddr, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
@@ -261,20 +280,20 @@ error_out:
*/
int setInterfaceName(const char* iface, const char* new)
{
- int rc = -1;
+ int rc;
const char *argv[] = {
"ip", "link", "set", iface, "name", new,
NULL
};
- int cmdResult;
-
- if (NULL == iface || NULL == new) {
- goto error_out;
- }
+ int cmdResult = 0;
rc = virRun(argv, &cmdResult);
- if (0 == rc)
- rc = cmdResult;
+ if (rc != 0 ||
+ (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
+ vethError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set '%s' to '%s' (%d)"),
+ new, iface, WEXITSTATUS(cmdResult));
+ rc = -1;
+ }
-error_out:
return rc;
}
diff --git a/src/lxc/veth.h b/src/lxc/veth.h
index 523bf9c..1ec1ec8 100644
--- a/src/lxc/veth.h
+++ b/src/lxc/veth.h
@@ -13,14 +13,21 @@
# define VETH_H
# include <config.h>
+# include "internal.h"
/* Function declarations */
int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
- int veth2MaxLen);
-int vethDelete(const char* veth);
-int vethInterfaceUpOrDown(const char* veth, int upOrDown);
-int moveInterfaceToNetNs(const char *iface, int pidInNs);
-int setMacAddr(const char* iface, const char* macaddr);
-int setInterfaceName(const char* iface, const char* new);
+ int veth2MaxLen)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+int vethDelete(const char* veth)
+ ATTRIBUTE_NONNULL(1);
+int vethInterfaceUpOrDown(const char* veth, int upOrDown)
+ ATTRIBUTE_NONNULL(1);
+int moveInterfaceToNetNs(const char *iface, int pidInNs)
+ ATTRIBUTE_NONNULL(1);
+int setMacAddr(const char* iface, const char* macaddr)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int setInterfaceName(const char* iface, const char* new)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
#endif /* VETH_H */
--
1.7.1.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1:10 p.m.
New subject: [libvirt] [PATCHv3] lxc: Fix return values of veth.c functions - complete
On 07/29/2010 01:53 PM, Ryota Ozaki wrote:
On Fri, Jul 30, 2010 at 2:32 AM, Laine Stump<laine(a)laine.org>
wrote:
> From: Ryota Ozaki<ozaki.ryota(a)gmail.com>
>
> Previously, the functions in src/lxc/veth.c could sometimes return
> positive values on failure rather than -1. This made accurate error
> reporting difficult, and led to one failure to catch an error in a
> calling function.
>
> This patch makes all the functions in veth.c consistently return 0 on
> success, and -1 on failure. It also fixes up the callers to the veth.c
> functions where necessary.
>
> Note that this patch may be related to the bug:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=607496.
>
> It will not fix the bug, but should unveil what happens.
>
> * po/POTFILES.in - add veth.c, which previously had no translatable strings
> * src/lxc/lxc_controller.c
> * src/lxc/lxc_container.c
> * src/lxc/lxc_driver.c - fixup callers to veth.c, and remove error logs,
> as they are now done in veth.c
> * src/lxc/veth.c - make all functions consistently return -1 on error.
> * src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args.
> ---
> Changes from Ozaki's original patch:
>
> 1) virAsprintf() will return the number of characters in the new
> string on success, not 0, so we need to only set rc if it fails
> (< 0). Assigning rc on success causes the caller to falsely believe
> the function failed.
>
> 2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even
> if waitpid had failed. I don't know if the behavior of WIFEXITED
> is defined if waitpid fails, but all the other uses I can find
> avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's
> what I did here. Also changed to return -1 if waitpid fails or
> WEXITSTATUS != 0.
>
> 3) lxcSetupInterfaces - rather than explicitly setting rc from the
> return of functions, since it defaults to -1, I just goto
> error_exit if the functions return< 0. That's just cosmetic. The
> real problem is that rc was being set from brAddInterface, which
> returns> 0 on failure (note error log of errno from brAddInterface
> fixed in v3).
>
> 4) assigning "rc = -1" at the beginning of each veth.c function is a
> dead store, since it will always be set by the call to virRun(). This
> causes coverity code analysis tool to report problems.
ACK. Thanks, Laine!
Pushed.
5231
days inactive
5237
days old
19 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Laine Stump -
Ryota Ozaki