On Thu, Feb 16, 2012 at 11:50:43AM +0100, Radek Vykydal wrote:
> On 02/15/2012 07:50 PM, David Cantrell wrote:
> >Any network interface configured by dracut on s390x is destroyed when
> >loader starts up with a graphical interface override flag passed on
> >the boot command line (e.g., 'vnc'). We call kickstartNetworkUp(),
> >which writes out ifcfg files to down everything and bring up the
> >interface the user wants.
> >
>
> I am trying to understand what is happening here:
>
> So dracut configures and brings up (which is new thing?) a network device.
> Loader then tries to start NetworkManager. From your patchset
> this thing doesn't seem as a problem, I wonder if it takes
> control over the devices (even whether NM_CONTROLLED is yes),
> but I suppose so.
It does.
> The problem seems to be that the condition in kickstartNetworkUp()
>
> if ((is_nm_connected() == TRUE) &&
> (loaderData->netDev != NULL) && (loaderData->netDev_set == 1))
> return 0;
>
> doesn't prevent the device activated in dracut from reactivating.
> Shouldn't we better fix this condition for s390x rather than remove
> the stuff all over for the arch?
>
> Now why the condition would fail? - I don't think that it is because
> of is_nm_connected() - if the active device is controlled by NM it should be
> detected (if it is not controlled I imagine it would hit us later).
>
> So is it because of the flags? We used to set them for s390 case
> in readNetInfo() you removed in related
> 471be3d8abebb8565965e0d320c71ead74a7399.
> Shouldn't we keep it in? Well, the configuration is written by dracut
> instead of linuxrc.s390, but this does not matter, we still need
> to suck pre-configuration (be it from linuxrc.s390 or dracut) in
> loader somehow.
>
> To sum up: to me it seems that readNetInfo shouldn't have been removed,
> then the loaderData->netDev and loaderData->netDev_set flags would
> be set properly and the condition in kickstartNetworkUp would prevent
> device reactivation.
Yeah, that is one way to handle the problem and I did explore that angle,
but removal is far easier. If readNetInfo() was to stay, then linuxrc.s390
needed to learn how to read and parse dracut configuration data so it could
then write out /tmp/s390net. Unfortunately dracut does not currently
write out things like SUBCHANNELS and stuff like that, so if I wanted to
make it write /tmp/s390net as it had been, it would have had to add more
code to linuxrc.s390 to do that.
And really, all of that is a pointless exercise because dracut has already
taken care of configuring the interface. This is why I chose the route of
removal.
And that is why I put in all the #if's in loader's code to prevent certain
steps from running on s390x because the iface_t struct is not populated
correctly. I would rather not waste time teaching linuxrc.s390 how to
read and parse dracut options -or- teach loader how to read and parse
dracut options because both linuxrc.s390 and loader are going away.
The primary goal of this patchset is to just prop up the code on the
rhel7-alpha-branch enough to run and be moderately useful. All of this
is throw-away anyway.
> (I am not sure if activating the device in dracut itself would not cause
> problems when NM is starting and if it takes it over neatly, but from your
> patches it seems that this should work ok and the problems come later.)
There are a number of issues, but with the patchset posted, users can
perform text mode or graphical installs with or without kickstart. Which
is really sufficient for now.
> Also I have one comment below:
>
>
> >In dracut-land, all of that has already happened, so suppress these
> >steps on s390x.
> >
> >NOTE: This is all taken care on master since loader has been removed.
> >
> >Related: rhbz#783227
> >---
> > loader/loader.c | 39 ++++++++++++++++++---------------------
> > loader/net.c | 2 ++
> > loader/nfsinstall.c | 2 ++
> > loader/urlinstall.c | 2 ++
> > 4 files changed, 24 insertions(+), 21 deletions(-)
> >
> >diff --git a/loader/loader.c b/loader/loader.c
> >index 75ffe80..f020366 100644
> >--- a/loader/loader.c
> >+++ b/loader/loader.c
> >@@ -194,10 +194,12 @@ void doGdbserver(struct loaderData_s *loaderData) {
> > pid_t loaderPid = getpid();
> > iface_init_iface_t(&iface);
> >
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > if (kickstartNetworkUp(loaderData,&iface)) {
> > logMessage(ERROR, "can't run gdbserver due to no network");
> > return;
> > }
> >+#endif
> >
> > checked_asprintf(&pid, "%d", loaderPid);
> >
> >@@ -750,9 +752,11 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData) {
> > }
> > } else if (!strcasecmp(k, "noeject")) {
> > flags |= LOADER_FLAGS_NOEJECT;
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > } else if (!strcasecmp(k, "sshd")) {
> > logMessage(INFO, "early networking required for sshd");
> > flags |= LOADER_FLAGS_EARLY_NETWORKING;
> >+#endif
> > } else if (!strcasecmp(k, "noverifyssl")) {
> > flags |= LOADER_FLAGS_NOVERIFYSSL;
> > } else if (v != NULL) {
> >@@ -916,6 +920,7 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData) {
> > * by loader, so an active connection is ready once we get
> > * to anaconda
> > */
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > if (!strcasecmp(k, "syslog") || !strcasecmp(k, "vnc")) {
> > logMessage(INFO, "early networking required for %s", k);
> > flags |= LOADER_FLAGS_EARLY_NETWORKING;
> >@@ -925,6 +930,7 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData) {
> > logMessage(INFO, "early networking required for remote kickstart configuration");
> > flags |= LOADER_FLAGS_EARLY_NETWORKING;
> > }
> >+#endif
> >
> > if (v != NULL) {
> > checked_asprintf(&extraArgs[numExtraArgs], "--%s=%s", k, v)
> >@@ -1356,12 +1362,17 @@ static void doLoaderMain(struct loaderData_s *loaderData,
> > if (loaderData->ksFile)
> > flags |= LOADER_FLAGS_IS_KICKSTART;
> >
> >- if (FL_HAVE_CMSCONF(flags)) {
> >+#if defined(__s390__) || defined(__s390x__)
> >+ {
> > loaderData->ipinfo_set = 1;
> > #ifdef ENABLE_IPV6
> > loaderData->ipv6info_set = 1;
> > #endif
> >+ step = STEP_EXTRAS;
> >+ dir = 1;
> >+ break;
> > }
> >+#endif
> >
> > rc = chooseNetworkInterface(loaderData);
> > if (rc == LOADER_BACK || rc == LOADER_ERROR || (dir == -1&& rc == LOADER_NOOP)) {
> >@@ -1392,15 +1403,6 @@ static void doLoaderMain(struct loaderData_s *loaderData,
> > doExit(EXIT_FAILURE);
> > }
> >
> >-
> >- /* s390 provides all config info by way of linuxrc.s390 */
> >- if (FL_HAVE_CMSCONF(flags)) {
> >- loaderData->ipinfo_set = 1;
> >-#ifdef ENABLE_IPV6
> >- loaderData->ipv6info_set = 1;
> >-#endif
> >- }
> >-
> > /* populate netDev based on any kickstart data */
> > logMessage(DEBUGLVL, "in doLoaderMain, calling setupIfaceStruct()");
> > setupIfaceStruct(&iface, loaderData);
> >@@ -1745,7 +1747,9 @@ int main(int argc, char ** argv) {
> > FILE *f;
> >
> > moduleInfoSet modInfo;
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > iface_t iface;
> >+#endif
> >
> > char ** argptr, ** tmparg;
> > char * anacondaArgs[50];
> >@@ -1844,8 +1848,10 @@ int main(int argc, char ** argv) {
> > logMessage(INFO, "text mode forced due to serial/virtpconsole");
> > flags |= LOADER_FLAGS_TEXT;
> > }
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > if (hasGraphicalOverride())
> > flags |= LOADER_FLAGS_EARLY_NETWORKING;
> >+#endif
> > set_fw_search_path(&loaderData, "/firmware:/lib/firmware");
> > start_fw_loader(&loaderData);
> >
> >@@ -1994,17 +2000,6 @@ int main(int argc, char ** argv) {
> > mlFreeModuleState(moduleState);
> > busProbe(FL_NOPROBE(flags));
> >
> >- /* Disable all network interfaces in NetworkManager by default */
> >-#if !defined(__s390__)&& !defined(__s390x__)
> >- {
> >- int i;
> >-
> >- if ((i = writeDisabledNetInfo()) != 0) {
> >- logMessage(ERROR, "writeDisabledNetInfo failure: %d", i);
> >- }
> >- }
> >-#endif
> >-
>
> Why to remove this, especially when it is guarded to not happen on s390?
> We need this in other archs.z
I think I got a little over ambitious here. I'll add it back.
> > #if defined(__s390__) || defined(__s390x__)
> > /* Start NetworkManager until we have systemd init on s390 too */
> > if (iface_start_NetworkManager())
> >@@ -2040,9 +2035,11 @@ int main(int argc, char ** argv) {
> > outputKSFile = runKickstart(&loaderData, loaderData.ksFile);
> > }
> >
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > if (FL_EARLY_NETWORKING(flags)) {
> > kickstartNetworkUp(&loaderData,&iface);
> > }
> >+#endif
> >
> > doLoaderMain(&loaderData, modInfo);
> > #ifdef USESELINUX
> >diff --git a/loader/net.c b/loader/net.c
> >index f8843c5..9ed8ec9 100644
> >--- a/loader/net.c
> >+++ b/loader/net.c
> >@@ -1755,6 +1755,7 @@ int chooseNetworkInterface(struct loaderData_s * loaderData) {
> > return LOADER_OK;
> > }
> >
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > /* JKFIXME: bad name. this function brings up networking early on a
> > * kickstart install so that we can do things like grab the ks.cfg from
> > * the network */
> >@@ -1797,6 +1798,7 @@ int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
> >
> > return activateDevice(loaderData, iface);
> > }
> >+#endif
> >
> > int disconnectDevice(char *device) {
> > int rc;
> >diff --git a/loader/nfsinstall.c b/loader/nfsinstall.c
> >index eb9579f..a52234e 100644
> >--- a/loader/nfsinstall.c
> >+++ b/loader/nfsinstall.c
> >@@ -325,10 +325,12 @@ int getFileFromNfs(char * url, char * dest, struct loaderData_s * loaderData) {
> > NMState state;
> > const GPtrArray *devices;
> >
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > if (kickstartNetworkUp(loaderData,&iface)) {
> > logMessage(ERROR, "unable to bring up network");
> > return 1;
> > }
> >+#endif
> >
> > /* if they just did 'linux ks', they want us to figure it out from
> > * the dhcp/bootp information
> >diff --git a/loader/urlinstall.c b/loader/urlinstall.c
> >index f3713d0..7594895 100644
> >--- a/loader/urlinstall.c
> >+++ b/loader/urlinstall.c
> >@@ -231,10 +231,12 @@ int getFileFromUrl(char * url, char * dest,
> >
> > iface_init_iface_t(&iface);
> >
> >+#if !defined(__s390__)&& !defined(__s390x__)
> > if (kickstartNetworkUp(loaderData,&iface)) {
> > logMessage(ERROR, "unable to bring up network");
> > return 1;
> > }
> >+#endif
> >
> > logMessage(INFO, "file location: %s", url);
> >
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--
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
