[libvirt] [PATCH] lxc: Fix return value handlings of vethInterfaceUpOrDown and moveInterfaceToNetNs
Ryota Ozaki
ozaki.ryota at gmail.com
Wed Jul 28 22:22:16 UTC 2010
On Wed, Jul 28, 2010 at 10:15 AM, Laine Stump <laine at 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 at 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 at 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 */
More information about the libvir-list
mailing list