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

[David Cantrell <dcantrell@xxxxxxxxxx>]

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@xxxxxxxxxx>
Supervisor, Installer Engineering Team
Red Hat, Inc. | Westford, MA | EST5EDT

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