[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH master, f16alpha nth] Restart NetworkManager to use anaconda's initial ifcfg config (#727951)



On 08/05/2011 08:51 AM, Radek Vykydal wrote:
On 08/05/2011 02:06 PM, David Cantrell wrote:
Comments below.

On 08/05/2011 04:47 AM, Radek Vykydal wrote:
NM is started by systemd before anaconda writes out initial ifcfg files
which results in creating of automatic default connections (e.g.
"Wired connection 1"). The settings of auto connections are not
persistent
so we are ok just restarting the NM.


---
loader/loader.c | 7 +++--
pyanaconda/isys/iface.c | 63
++++++++++------------------------------------
pyanaconda/isys/iface.h | 4 +-
3 files changed, 20 insertions(+), 54 deletions(-)

diff --git a/loader/loader.c b/loader/loader.c
index ec07b78..5b87ba8 100644
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -2173,9 +2173,10 @@ int main(int argc, char ** argv) {
}
#endif

- /* Start NetworkManager now so it's always available to talk to. */
- if (iface_start_NetworkManager())
- logMessage(INFO, "failed to start NetworkManager");
+ /* Restart NetworkManager now so that it uses our inital ifcfg
config */
+ logMessage(INFO, "restarting NetworkManager");
+ if (iface_restart_NetworkManager())
+ logMessage(ERROR, "failed to restart NetworkManager");

if (!FL_CMDLINE(flags))
startNewt();
diff --git a/pyanaconda/isys/iface.c b/pyanaconda/isys/iface.c
index b5d539a..cdb1141 100644
--- a/pyanaconda/isys/iface.c
+++ b/pyanaconda/isys/iface.c
@@ -58,7 +58,6 @@
static struct nl_handle *_iface_get_handle(void);
static struct nl_cache *_iface_get_link_cache(struct nl_handle **);
static int _iface_have_valid_addr(void *addr, int family, int length);
-static int _iface_redirect_io(char *device, int fd, int mode);

/*
* Return a libnl handle for NETLINK_ROUTE.
@@ -128,31 +127,6 @@ static int _iface_have_valid_addr(void *addr,
int family, int length) {
}

/*
- * Redirect I/O to another device (e.g., stdout to /dev/tty5)
- */
-int _iface_redirect_io(char *device, int fd, int mode) {
- int io = -1;
-
- if ((io = open(device, mode)) == -1) {
- return 1;
- }
-
- if (close(fd) == -1) {
- return 2;
- }
-
- if (dup2(io, fd) == -1) {
- return 3;
- }
-
- if (close(io) == -1) {
- return 4;
- }
-
- return 0;
-}
-
-/*
* Given an interface name (e.g., eth0) and address family (e.g.,
AF_INET),
* return the IP address in human readable format (i.e., the output from
* inet_ntop()). Return NULL for no match or error.
@@ -498,40 +472,31 @@ int wait_for_nm(void) {
}

/*
- * Start NetworkManager -- requires that you have already written
out the
+ * Restart NetworkManager -- requires that you have already written
out the
* control files in /etc/sysconfig for the interface.
*/
-int iface_start_NetworkManager(void) {
- pid_t pid;
+int iface_restart_NetworkManager(void) {
+ int child, status;

- if (is_nm_running())
- return 0; /* already running */
+ if (!(child = fork())) {
+ int fd = open("/dev/tty3", O_RDWR);

- /* Start NetworkManager */
- pid = fork();
- if (pid == 0) {
- if (setpgrp() == -1) {
- exit(1);
- }
+ dup2(fd, 0);
+ dup2(fd, 1);
+ dup2(fd, 2);
+ close(fd);

- if (_iface_redirect_io("/dev/null", STDIN_FILENO, O_RDONLY) ||
- _iface_redirect_io(OUTPUT_TERMINAL, STDOUT_FILENO, O_WRONLY) ||
- _iface_redirect_io(OUTPUT_TERMINAL, STDERR_FILENO, O_WRONLY)) {
- exit(2);
- }
+ execl("/bin/systemctl", "/bin/systemctl", "restart",
"NetworkManager.service", NULL);
+ _exit(1);
+ }

You've dropped the return value checking for dup2() and execl(), among
other things. Please add these back, as well as checking return values
for other functions you call (e.g., waitpid()). _iface_redirect_io()
was meant to make that easier for dup2().


Thanks for the review.

I thought that checking errors of redirection (dup2) is not so important
when we fork/exec only systemctl that starts NM, not the NM itself,
also we use dup2 without error checking happily in loader
but ok, I will use _iface_redirect_io.

It's not so important in that it's probably not likely to fail unless there are other problems, but it's bad form.

It's similar to the patch we recently accepted where we were free()'ing something in loader but then not setting that pointer to NULL. Through several subsequent patches, that buffer was used again but when it wasn't NULL and passed to a function that allocated memory...bust.

I'll also add checking of waitpid.

Checking return value of execl doesn't make sense, if it returns
there was an error.

Right, wouldn't we want a log message indicating we couldn't exec what we wanted to exec? Or maybe some other action?

I'll post new version of the patch.

Thanks,

--
David Cantrell <dcantrell redhat com>
Supervisor, Installer Engineering Team
Red Hat, Inc. | Westford, MA | EST5EDT


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]