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

[Radek Vykydal <rvykydal@xxxxxxxxxx>]

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.

I'll also add checking of waitpid.

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

I'll post new version of the patch.

Radek

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list