RHEL-5 patch to fix #448006

[David Cantrell]

Regression introduced by me for the fix for #432011, this patch makes sure
we grab the bootfile from the DHCP response.

Tested locally using a RHEL 5.3 tree.  The 'ks' argument works with this
patch in place.

[rhel5-branch] Save bootfile, if we have it, from DHCP response (#448006)

[David Cantrell]

Saves the bootfile if we get one so that it can be used later to
construct the default kickstart file to try if the user boots with 'ks'
as a boot parameter.
---
 loader2/net.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/loader2/net.c b/loader2/net.c
index fbbde60..d25c1aa 100644
--- a/loader2/net.c
+++ b/loader2/net.c
@@ -1532,12 +1532,14 @@ int doDhcp(struct networkDeviceConfig *dev) {
     char *r = NULL, *class = NULL;
     char namebuf[HOST_NAME_MAX];
     time_t timeout;
-    int loglevel, status, ret = 0, i;
+    int loglevel, status, ret = 0, i, sz = 0;
     int shmpump;
     DHCP_Preference pref = 0;
     pid_t pid;
     key_t key;
     int mturet;
+    int culvert[2];
+    char buf[PATH_MAX];

/* clear existing IP addresses */
     clearInterface(dev->dev.device);
@@ -1602,9 +1604,16 @@ int doDhcp(struct networkDeviceConfig *dev) {

strncpy(pumpdev->device, dev->dev.device, IF_NAMESIZE);

+        if (pipe(culvert) == -1) {
+            logMessage(ERROR, "%s: pipe(): %s", __func__, strerror(errno));
+            return 1;
+        }
+
         /* call libdhcp in a separate process because libdhcp is bad */
         pid = fork();
         if (pid == 0) {
+            close(culvert[0]);
+
             if (pumpdev->set & PUMP_INTFINFO_HAS_MTU) {
                 mturet = nl_set_device_mtu((char *) pumpdev->device, pumpdev->mtu);

@@ -1658,14 +1667,25 @@ int doDhcp(struct networkDeviceConfig *dev) {
                 }
             }

-            /* we lose bootFile here, but you know, I just don't care, because
-             * we don't need it past doDhcp() calls, so whatever   --dcantrell
-             */
+            if (pumpdev->set & PUMP_INTFINFO_HAS_BOOTFILE) {
+                if (pumpdev->bootFile) {
+                    if (write(culvert[1], pumpdev->bootFile,
+                              strlen(pumpdev->bootFile) + 1) == -1) {
+                        logMessage(ERROR, "failed to send bootFile to parent "
+                                          "in %s: %s", __func__,
+                                   strerror(errno));
+                    }
+
+                    close(culvert[1]);
+                }
+            }

exit(0);
         } else if (pid == -1) {
             logMessage(CRITICAL, "dhcp client failed to start");
         } else {
+            close(culvert[1]);
+
             if (waitpid(pid, &status, 0) == -1) {
                 logMessage(ERROR, "waitpid() failure in %s", __func__);
             }
@@ -1733,6 +1753,34 @@ int doDhcp(struct networkDeviceConfig *dev) {
                 }
             }

+            if (dev->dev.set & PUMP_INTFINFO_HAS_BOOTFILE) {
+                memset(&buf, '\0', sizeof(buf));
+
+                while ((sz = read(culvert[0], &buf, sizeof(buf))) > 0) {
+                    if (dev->dev.bootFile == NULL) {
+                        dev->dev.bootFile = calloc(sizeof(char), sz + 1);
+                        if (dev->dev.bootFile == NULL) {
+                            logMessage(ERROR, "unable to read bootfile");
+                            break;
+                        }
+
+                        dev->dev.bootFile = strncpy(dev->dev.bootFile, buf, sz);
+                    } else {
+                        dev->dev.bootFile = realloc(dev->dev.bootFile,
+                                                    strlen(dev->dev.bootFile) +
+                                                    sz + 1);
+                        if (dev->dev.bootFile == NULL) {
+                            logMessage(ERROR, "unable to read bootfile");
+                            break;
+                        }
+
+                        dev->dev.bootFile = strncat(dev->dev.bootFile, buf, sz);
+                    }
+                }
+
+                close(culvert[0]);
+            }
+
             if (shmdt(pumpdev) == -1) {
                 logMessage(ERROR, "%s: shmdt() pumpdev: %s", __func__,
                            strerror(errno));

[rhel5-branch] Save bootfile, if we have it, from DHCP response (#448006)

[Hans de Goede]

Hi,

Comments inline.

On 07/01/2009 05:08 AM, David Cantrell wrote:
> Saves the bootfile if we get one so that it can be used later to
> construct the default kickstart file to try if the user boots with 'ks'
> as a boot parameter.
>
> loader2/net.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>   1 files changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/loader2/net.c b/loader2/net.c
> index fbbde60..d25c1aa 100644
> --- a/loader2/net.c
> +++ b/loader2/net.c
> @@ -1532,12 +1532,14 @@ int doDhcp(struct networkDeviceConfig *dev) {
>       char *r = NULL, *class = NULL;
>       char namebuf[HOST_NAME_MAX];
>       time_t timeout;
> int loglevel, status, ret = 0, i;
>
> int loglevel, status, ret = 0, i, sz = 0;
>  int shmpump;
>  DHCP_Preference pref = 0;
>  pid_t pid;
>  key_t key;
>  int mturet;
>
> int culvert[2];
>
> char buf[PATH_MAX];
>
> /* clear existing IP addresses */
>  clearInterface(dev->dev.device);
>
> @@ -1602,9 +1604,16 @@ int doDhcp(struct networkDeviceConfig *dev) {
>       strncpy(pumpdev->device, dev->dev.device, IF_NAMESIZE);
>
>    if (pipe(culvert) == -1) {
>        logMessage(ERROR, "%s: pipe(): %s", __func__, strerror(errno));
>        return 1;
>    }
>     /* call libdhcp in a separate process because libdhcp is bad */
>     pid = fork();
>     if (pid == 0) {
>        close(culvert[0]);
>         if (pumpdev->set&  PUMP_INTFINFO_HAS_MTU) {
>             mturet = nl_set_device_mtu((char *) pumpdev->device, pumpdev->mtu);
>
> @@ -1658,14 +1667,25 @@ int doDhcp(struct networkDeviceConfig *dev) {
>                   }
>               }
>        /* we lose bootFile here, but you know, I just don't care, because
>         * we don't need it past doDhcp() calls, so whatever   --dcantrell
>         */
>        if (pumpdev->set&  PUMP_INTFINFO_HAS_BOOTFILE) {
>            if (pumpdev->bootFile) {
>                if (write(culvert[1], pumpdev->bootFile,
>                          strlen(pumpdev->bootFile) + 1) == -1) {
>                    logMessage(ERROR, "failed to send bootFile to parent "
>                                      "in %s: %s", __func__,
>                               strerror(errno));
>                }
>                close(culvert[1]);
>            }
>        }
>
>         exit(0);

culvert[1] should be closed even when there is no bootfile, not that it matters much
as we do an exit(0) right afterwards, still the "close(culvert[1]);" should be
outside the if block.
>       } else if (pid == -1) {
>           logMessage(CRITICAL, "dhcp client failed to start");
>       } else {
>        close(culvert[1]);
>         if (waitpid(pid,&status, 0) == -1) {
>             logMessage(ERROR, "waitpid() failure in %s", __func__);
>         }
>
> @@ -1733,6 +1753,34 @@ int doDhcp(struct networkDeviceConfig *dev) {
>                   }
>               }
>        if (dev->dev.set&  PUMP_INTFINFO_HAS_BOOTFILE) {
>            memset(&buf, '\0', sizeof(buf));
>            while ((sz = read(culvert[0],&buf, sizeof(buf)))>  0) {
>                if (dev->dev.bootFile == NULL) {
>                    dev->dev.bootFile = calloc(sizeof(char), sz + 1);
>                    if (dev->dev.bootFile == NULL) {
>                        logMessage(ERROR, "unable to read bootfile");
>                        break;
>                    }
>                    dev->dev.bootFile = strncpy(dev->dev.bootFile, buf, sz);
>                } else {
>                    dev->dev.bootFile = realloc(dev->dev.bootFile,
>                                                strlen(dev->dev.bootFile) +
>                                                sz + 1);
>                    if (dev->dev.bootFile == NULL) {
>                        logMessage(ERROR, "unable to read bootfile");
>                        break;
>                    }
>                    dev->dev.bootFile = strncat(dev->dev.bootFile, buf, sz);
>                }
>            }
>            close(culvert[0]);
>        }

Same for closing culvert[0] here, that should be outside the if block and here it does
matter.

Also I wonder could this get called twice on the same dev ? It might be a good idea to
free (*) dev->dev.bootFile and then set it to NULL before the while, to make sure we
are not appending a new bootFile string to an old one, instead of starting a new.
>           if (shmdt(pumpdev) == -1) {
>               logMessage(ERROR, "%s: shmdt() pumpdev: %s", __func__,
>                          strerror(errno));

Other then that it looks ok,

Regards,

Hans

*) Remember free() on a NULL pointer is a no-op so perfectly safe to do.

[rhel5-branch] Save bootfile, if we have it, from DHCP response (#448006)

[David Cantrell]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Wed, 1 Jul 2009, Hans de Goede wrote:
> Hi,
>
> Comments inline.
>
> On 07/01/2009 05:08 AM, David Cantrell wrote:
> > Saves the bootfile if we get one so that it can be used later to
> > construct the default kickstart file to try if the user boots with 'ks'
> > as a boot parameter.
> >
> > loader2/net.c |   56 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >   1 files changed, 52 insertions(+), 4 deletions(-)
> >
> > diff --git a/loader2/net.c b/loader2/net.c
> > index fbbde60..d25c1aa 100644
> > --- a/loader2/net.c
> > +++ b/loader2/net.c
> > @@ -1532,12 +1532,14 @@ int doDhcp(struct networkDeviceConfig *dev) {
> >       char *r = NULL, *class = NULL;
> >       char namebuf[HOST_NAME_MAX];
> >       time_t timeout;
> > int loglevel, status, ret = 0, i;
> >
> > int loglevel, status, ret = 0, i, sz = 0;
> >  int shmpump;
> >  DHCP_Preference pref = 0;
> >  pid_t pid;
> >  key_t key;
> >  int mturet;
> >
> > int culvert[2];
> >
> > char buf[PATH_MAX];
> >
> > /* clear existing IP addresses */
> >  clearInterface(dev->dev.device);
> >
> > @@ -1602,9 +1604,16 @@ int doDhcp(struct networkDeviceConfig *dev) {
> >       strncpy(pumpdev->device, dev->dev.device, IF_NAMESIZE);
> >
> >    if (pipe(culvert) == -1) {
> >        logMessage(ERROR, "%s: pipe(): %s", __func__, 
> >
> > strerror(errno));
> >        return 1;
> >    }
> >     /* call libdhcp in a separate process because libdhcp is bad */
> >     pid = fork();
> >     if (pid == 0) {
> >        close(culvert[0]);
> >         if (pumpdev->set&  PUMP_INTFINFO_HAS_MTU) {
> >             mturet = nl_set_device_mtu((char *) pumpdev->device, 
> >
> > pumpdev->mtu);
> >
> > @@ -1658,14 +1667,25 @@ int doDhcp(struct networkDeviceConfig *dev) {
> >                   }
> >               }
> >        /* we lose bootFile here, but you know, I just don't care, 
> >
> > because
> >         * we don't need it past doDhcp() calls, so whatever 
> >
> > --dcantrell
> >         */
> >        if (pumpdev->set&  PUMP_INTFINFO_HAS_BOOTFILE) {
> >            if (pumpdev->bootFile) {
> >                if (write(culvert[1], pumpdev->bootFile,
> >                          strlen(pumpdev->bootFile) + 1) == -1) {
> >                    logMessage(ERROR, "failed to send bootFile to 
> >
> > parent "
> >                                      "in %s: %s", __func__,
> >                               strerror(errno));
> >                }
> >                close(culvert[1]);
> >            }
> >        }
> >
> >         exit(0);
>
> culvert[1] should be closed even when there is no bootfile, not that it 
> matters much
> as we do an exit(0) right afterwards, still the "close(culvert[1]);" should 
> be
> outside the if block.

Ooops, right.  Good catch.
> >       } else if (pid == -1) {
> >           logMessage(CRITICAL, "dhcp client failed to start");
> >       } else {
> >        close(culvert[1]);
> >         if (waitpid(pid,&status, 0) == -1) {
> >             logMessage(ERROR, "waitpid() failure in %s", __func__);
> >         }
> >
> > @@ -1733,6 +1753,34 @@ int doDhcp(struct networkDeviceConfig *dev) {
> >                   }
> >               }
> >        if (dev->dev.set&  PUMP_INTFINFO_HAS_BOOTFILE) {
> >            memset(&buf, '\0', sizeof(buf));
> >            while ((sz = read(culvert[0],&buf, sizeof(buf)))>  0) {
> >                if (dev->dev.bootFile == NULL) {
> >                    dev->dev.bootFile = calloc(sizeof(char), sz + 1);
> >                    if (dev->dev.bootFile == NULL) {
> >                        logMessage(ERROR, "unable to read bootfile");
> >                        break;
> >                    }
> >                    dev->dev.bootFile = strncpy(dev->dev.bootFile, 
> >
> > buf, sz);
> >                } else {
> >                    dev->dev.bootFile = realloc(dev->dev.bootFile,
> >
> > strlen(dev->dev.bootFile) +
> >                                                sz + 1);
> >                    if (dev->dev.bootFile == NULL) {
> >                        logMessage(ERROR, "unable to read bootfile");
> >                        break;
> >                    }
> >                    dev->dev.bootFile = strncat(dev->dev.bootFile, 
> >
> > buf, sz);
> >                }
> >            }
> >            close(culvert[0]);
> >        }
>
> Same for closing culvert[0] here, that should be outside the if block and 
> here it does
> matter.
>
> Also I wonder could this get called twice on the same dev ? It might be a 
> good idea to
> free (*) dev->dev.bootFile and then set it to NULL before the while, to make 
> sure we
> are not appending a new bootFile string to an old one, instead of starting a 
> new.

While I'm inclined to say that we won't hit this twice, the way it's currently
written is that bootFile would just grow if we kept hitting the code, so
better safe than less safe.

Updated patch:

- ---
  loader2/net.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
  1 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/loader2/net.c b/loader2/net.c
index fbbde60..bc4973e 100644
- --- a/loader2/net.c
+++ b/loader2/net.c
@@ -1532,12 +1532,14 @@ int doDhcp(struct networkDeviceConfig *dev) {
      char *r = NULL, *class = NULL;
      char namebuf[HOST_NAME_MAX];
      time_t timeout;
- -    int loglevel, status, ret = 0, i;
+    int loglevel, status, ret = 0, i, sz = 0;
      int shmpump;
      DHCP_Preference pref = 0;
      pid_t pid;
      key_t key;
      int mturet;
+    int culvert[2];
+    char buf[PATH_MAX];

/* clear existing IP addresses */
      clearInterface(dev->dev.device);
@@ -1602,9 +1604,16 @@ int doDhcp(struct networkDeviceConfig *dev) {

strncpy(pumpdev->device, dev->dev.device, IF_NAMESIZE);

+        if (pipe(culvert) == -1) {
+            logMessage(ERROR, "%s: pipe(): %s", __func__, strerror(errno));
+            return 1;
+        }
+
          /* call libdhcp in a separate process because libdhcp is bad */
          pid = fork();
          if (pid == 0) {
+            close(culvert[0]);
+
              if (pumpdev->set & PUMP_INTFINFO_HAS_MTU) {
                  mturet = nl_set_device_mtu((char *) pumpdev->device, pumpdev->mtu);

@@ -1658,14 +1667,24 @@ int doDhcp(struct networkDeviceConfig *dev) {
                  }
              }

- -            /* we lose bootFile here, but you know, I just don't care, because
- -             * we don't need it past doDhcp() calls, so whatever   --dcantrell
- -             */
+            if (pumpdev->set & PUMP_INTFINFO_HAS_BOOTFILE) {
+                if (pumpdev->bootFile) {
+                    if (write(culvert[1], pumpdev->bootFile,
+                              strlen(pumpdev->bootFile) + 1) == -1) {
+                        logMessage(ERROR, "failed to send bootFile to parent "
+                                          "in %s: %s", __func__,
+                                   strerror(errno));
+                    }
+                }
+            }

+            close(culvert[1]);
              exit(0);
          } else if (pid == -1) {
              logMessage(CRITICAL, "dhcp client failed to start");
          } else {
+            close(culvert[1]);
+
              if (waitpid(pid, &status, 0) == -1) {
                  logMessage(ERROR, "waitpid() failure in %s", __func__);
              }
@@ -1733,6 +1752,36 @@ int doDhcp(struct networkDeviceConfig *dev) {
                  }
              }

+            if (dev->dev.set & PUMP_INTFINFO_HAS_BOOTFILE) {
+                memset(&buf, '\0', sizeof(buf));
+                free(dev->dev.bootFile);
+                dev->dev.bootFile = NULL;
+
+                while ((sz = read(culvert[0], &buf, sizeof(buf))) > 0) {
+                    if (dev->dev.bootFile == NULL) {
+                        dev->dev.bootFile = calloc(sizeof(char), sz + 1);
+                        if (dev->dev.bootFile == NULL) {
+                            logMessage(ERROR, "unable to read bootfile");
+                            break;
+                        }
+
+                        dev->dev.bootFile = strncpy(dev->dev.bootFile, buf, sz);
+                    } else {
+                        dev->dev.bootFile = realloc(dev->dev.bootFile,
+                                                    strlen(dev->dev.bootFile) +
+                                                    sz + 1);
+                        if (dev->dev.bootFile == NULL) {
+                            logMessage(ERROR, "unable to read bootfile");
+                            break;
+                        }
+
+                        dev->dev.bootFile = strncat(dev->dev.bootFile, buf, sz);
+                    }
+                }
+            }
+
+            close(culvert[0]);
+
              if (shmdt(pumpdev) == -1) {
                  logMessage(ERROR, "%s: shmdt() pumpdev: %s", __func__,
                             strerror(errno));
- -- 
1.6.3.3

- -- 
David Cantrell 
Red Hat / Honolulu, HI
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkpLwYgACgkQ5hsjjIy1Vkn2+ACgsL25RU73HJpCTURAqSsWU79g
> xb0Anj+P3LbdoL4LOuKnWf5QrdgjrSu2
> =Ifl0
> -----END PGP SIGNATURE-----

[rhel5-branch] Save bootfile, if we have it, from DHCP response (#448006)

[Hans de Goede]

Hi,

On 07/01/2009 10:05 PM, David Cantrell wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Wed, 1 Jul 2009, Hans de Goede wrote:
> > Hi,
> >
> > Comments inline.
> >
> > On 07/01/2009 05:08 AM, David Cantrell wrote:
> > > Saves the bootfile if we get one so that it can be used later to
> > > construct the default kickstart file to try if the user boots with 'ks'
> > > as a boot parameter.
> > >
> > > loader2/net.c | 56
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > > 1 files changed, 52 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/loader2/net.c b/loader2/net.c
> > > index fbbde60..d25c1aa 100644
> > > --- a/loader2/net.c
> > > +++ b/loader2/net.c
> > > @@ -1532,12 +1532,14 @@ int doDhcp(struct networkDeviceConfig *dev) {
> > > char *r = NULL, *class = NULL;
> > > char namebuf[HOST_NAME_MAX];
> > > time_t timeout;
> > > int loglevel, status, ret = 0, i;
> > > int loglevel, status, ret = 0, i, sz = 0;
> > >
> > > int shmpump;
> > > DHCP_Preference pref = 0;
> > > pid_t pid;
> > > key_t key;
> > > int mturet;
> > > int culvert[2];
> > > char buf[PATH_MAX];
> > >
> > > /* clear existing IP addresses */
> > > clearInterface(dev->dev.device);
> > > @@ -1602,9 +1604,16 @@ int doDhcp(struct networkDeviceConfig *dev) {
> > >
> > > strncpy(pumpdev->device, dev->dev.device, IF_NAMESIZE);
> > > if (pipe(culvert) == -1) {
> > > logMessage(ERROR, "%s: pipe(): %s", __func__, strerror(errno));
> > > return 1;
> > > }
> > >
> > > /* call libdhcp in a separate process because libdhcp is bad */
> > > pid = fork();
> > > if (pid == 0) {
> > > close(culvert[0]);
> > >
> > > if (pumpdev->set& PUMP_INTFINFO_HAS_MTU) {
> > > mturet = nl_set_device_mtu((char *) pumpdev->device, pumpdev->mtu);
> > >
> > > @@ -1658,14 +1667,25 @@ int doDhcp(struct networkDeviceConfig *dev) {
> > > }
> > > }
> > > /* we lose bootFile here, but you know, I just don't care, because
> > > we don't need it past doDhcp() calls, so whatever --dcantrell
> > > */
> > > if (pumpdev->set& PUMP_INTFINFO_HAS_BOOTFILE) {
> > > if (pumpdev->bootFile) {
> > > if (write(culvert[1], pumpdev->bootFile,
> > > strlen(pumpdev->bootFile) + 1) == -1) {
> > > logMessage(ERROR, "failed to send bootFile to parent "
> > > "in %s: %s", __func__,
> > > strerror(errno));
> > > }
> > > close(culvert[1]);
> > > }
> > > }
> > >
> > > exit(0);
> >
> > culvert[1] should be closed even when there is no bootfile, not that
> > it matters much
> > as we do an exit(0) right afterwards, still the "close(culvert[1]);"
> > should be
> > outside the if block.
>
> Ooops, right. Good catch.
> > > } else if (pid == -1) {
> > > logMessage(CRITICAL, "dhcp client failed to start");
> > > } else {
> > > close(culvert[1]);
> > >
> > > if (waitpid(pid,&status, 0) == -1) {
> > > logMessage(ERROR, "waitpid() failure in %s", __func__);
> > > }
> > > @@ -1733,6 +1753,34 @@ int doDhcp(struct networkDeviceConfig *dev) {
> > > }
> > > }
> > > if (dev->dev.set& PUMP_INTFINFO_HAS_BOOTFILE) {
> > > memset(&buf, '\0', sizeof(buf));
> > > while ((sz = read(culvert[0],&buf, sizeof(buf)))> 0) {
> > > if (dev->dev.bootFile == NULL) {
> > > dev->dev.bootFile = calloc(sizeof(char), sz + 1);
> > > if (dev->dev.bootFile == NULL) {
> > > logMessage(ERROR, "unable to read bootfile");
> > > break;
> > > }
> > > dev->dev.bootFile = strncpy(dev->dev.bootFile, buf, sz);
> > > } else {
> > > dev->dev.bootFile = realloc(dev->dev.bootFile,
> > > strlen(dev->dev.bootFile) +
> > > sz + 1);
> > > if (dev->dev.bootFile == NULL) {
> > > logMessage(ERROR, "unable to read bootfile");
> > > break;
> > > }
> > > dev->dev.bootFile = strncat(dev->dev.bootFile, buf, sz);
> > > }
> > > }
> > > close(culvert[0]);
> > > }
> >
> > Same for closing culvert[0] here, that should be outside the if block
> > and here it does
> > matter.
> >
> > Also I wonder could this get called twice on the same dev ? It might
> > be a good idea to
> > free (*) dev->dev.bootFile and then set it to NULL before the while,
> > to make sure we
> > are not appending a new bootFile string to an old one, instead of
> > starting a new.
>
> While I'm inclined to say that we won't hit this twice, the way it's
> currently
> written is that bootFile would just grow if we kept hitting the code, so
> better safe than less safe.

Looks good now,

Regards,

Hans