Re: [master] Introduces CHECK_ASPRINTF macro that checks asprintfs return value and terminates program in OOM scenarios.

[David Cantrell <dcantrell@xxxxxxxxxx>]

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

I'm personally not a fan of using macros for more than constant values.
However, I can see this helping guarantee correct asprintf() usage.  I would
like to see:

1) The alignment of "\" on the ends of the macro lines corrected.

2) All asprintf() usages throughout loader changed, not just those in
   nfsinstall.c

On Fri, 13 Nov 2009, Ales Kozumplik wrote:

> This is to avoid having to copy-paste the asprintf-log-abort if branch all the time.
> ---
> loader/loader.h     |    6 +++
> loader/nfsinstall.c |  125 +++++++++++++--------------------------------------
> 2 files changed, 38 insertions(+), 93 deletions(-)
>
> diff --git a/loader/loader.h b/loader/loader.h
> index f942b7e..039c72a 100644
> --- a/loader/loader.h
> +++ b/loader/loader.h
> @@ -185,4 +185,10 @@ struct loaderData_s {
> #define LIBPATH "/lib:/usr/lib:/usr/X11R6/lib:/usr/kerberos/lib:/mnt/usr/lib:/mnt/sysimage/lib:/mnt/sysimage/usr/lib"
> #endif
>
> +#define CHECKED_ASPRINTF(...)                                       \
> +    if (asprintf( __VA_ARGS__ ) == -1) {                             \
> +        logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);     \
> +        abort();                                                    \
> +    }
> +
> #endif
> diff --git a/loader/nfsinstall.c b/loader/nfsinstall.c
> index e405df9..0eb3cbe 100644
> --- a/loader/nfsinstall.c
> +++ b/loader/nfsinstall.c
> @@ -68,24 +68,16 @@ static int nfsGetSetup(char ** hostptr, char ** dirptr) {
>     entries[0].text = _("NFS server name:");
>     entries[0].value = &newServer;
>     entries[0].flags = NEWT_FLAG_SCROLL;
> -
> -    if (asprintf(&entries[1].text, _("%s directory:"),
> -                 getProductName()) == -1) {
> -        logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -        abort();
> -    }
> +
> +    CHECKED_ASPRINTF(&entries[1].text, _("%s directory:"), getProductName());
>
>     entries[1].value = &newDir;
>     entries[1].flags = NEWT_FLAG_SCROLL;
>     entries[2].text = NULL;
>     entries[2].value = NULL;
>
> -    if (asprintf(&buf, _("Please enter the server name and path to your %s "
> -                         "installation image."), getProductName()) == -1) {
> -        logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -        abort();
> -    }
> -
> +    CHECKED_ASPRINTF(&buf, _("Please enter the server name and path to your %s "
> +                             "installation image."), getProductName());
>     do {
>         rc = newtWinEntries(_("NFS Setup"), buf, 60, 5, 15,
>                             24, entries, _("OK"), _("Back"), NULL);
> @@ -149,11 +141,7 @@ static void addDefaultKickstartFile(char **file, char *ip) {
>      */
>     if ((*file) && (((*file)[strlen(*file) - 1] == '/') ||
>                     ((*file)[strlen(*file) - 1] == '\0'))) {
> -        if (asprintf(file, "%s%s-kickstart", *file, ip) == -1) {
> -            logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -            abort();
> -        }
> -
> +        CHECKED_ASPRINTF(file, "%s%s-kickstart", *file, ip);
>         logMessage(DEBUGLVL, "addDefaultKickstartFile file: |%s|", *file);
>     }
> }
> @@ -184,12 +172,9 @@ char * mountNfsImage(struct installMethod * method,
>                     loaderData->stage2Data)->mountOpts == NULL) {
>                     mountOpts = strdup("ro");
>                 } else {
> -                    if (asprintf(&mountOpts, "ro,%s",
> -                                 ((struct nfsInstallData *)
> -                                 loaderData->stage2Data)->mountOpts) == -1) {
> -                        logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                        abort();
> -                    }
> +                    CHECKED_ASPRINTF(&mountOpts, "ro,%s",
> +                                     ((struct nfsInstallData *)
> +                                      loaderData->stage2Data)->mountOpts);
>                 }
>
>                 logMessage(INFO, "host is %s, dir is %s, opts are '%s'", host, directory, mountOpts);
> @@ -219,18 +204,10 @@ char * mountNfsImage(struct installMethod * method,
>                  */
>                 substr = strstr(directory, ".img");
>                 if (!substr || (substr && *(substr+4) != '\0')) {
> -                    if (asprintf(&(loaderData->instRepo), "nfs:%s:%s",
> -                                 host, directory) == -1) {
> -                        logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                        abort();
> -                    }
> -
> -                    if (asprintf(&tmp, "nfs:%s:%s/images/install.img",
> -                                 host, directory) == -1) {
> -                        logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                        abort();
> -                    }
> -
> +                    CHECKED_ASPRINTF(&(loaderData->instRepo), "nfs:%s:%s",
> +                                     host, directory);
> +                    CHECKED_ASPRINTF(&tmp, "nfs:%s:%s/images/install.img",
> +                                     host, directory);
>                     setStage2LocFromCmdline(tmp, loaderData);
>                     free(host);
>                     free(directory);
> @@ -247,13 +224,9 @@ char * mountNfsImage(struct installMethod * method,
>         case NFS_STAGE_MOUNT: {
>             char *buf;
>
> -            if (asprintf(&fullPath, "%s:%.*s", host,
> -                         (int) (strrchr(directory, '/')-directory),
> -                         directory) == -1) {
> -                logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                abort();
> -            }
> -
> +            CHECKED_ASPRINTF(&fullPath, "%s:%.*s", host,
> +                             (int) (strrchr(directory, '/')-directory),
> +                             directory);
>             logMessage(INFO, "mounting nfs path %s", fullPath);
>
>             if (FL_TESTING(flags)) {
> @@ -264,11 +237,8 @@ char * mountNfsImage(struct installMethod * method,
>             stage = NFS_STAGE_NFS;
>
>             if (!doPwMount(fullPath, "/mnt/stage2", "nfs", mountOpts, NULL)) {
> -                if (asprintf(&buf, "/mnt/stage2/%s",
> -                             strrchr(directory, '/')) == -1) {
> -                    logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                    abort();
> -                }
> +                CHECKED_ASPRINTF(&buf, "/mnt/stage2/%s",
> +                                 strrchr(directory, '/'));
>
>                 if (!access(buf, R_OK)) {
>                     logMessage(INFO, "can access %s", buf);
> @@ -276,14 +246,8 @@ char * mountNfsImage(struct installMethod * method,
>
>                     if (rc == 0) {
>                         stage = NFS_STAGE_UPDATES;
> -
> -                        if (asprintf(&url, "nfs:%s:%s", host,
> -                                     directory) == -1) {
> -                            logMessage(CRITICAL, "%s: %d: %m", __func__,
> -                                       __LINE__);
> -                            abort();
> -                        }
> -
> +                        CHECKED_ASPRINTF(&url, "nfs:%s:%s", host,
> +                                         directory);
>                         free(buf);
>                         break;
>                     } else {
> @@ -309,12 +273,10 @@ char * mountNfsImage(struct installMethod * method,
>                 break;
>             }
>
> -            if (asprintf(&buf, _("That directory does not seem to "
> -                                 "contain a %s installation image."),
> -                         getProductName()) == -1) {
> -                logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                abort();
> -            }
> +            CHECKED_ASPRINTF(&buf,
> +                             _("That directory does not seem to "
> +                               "contain a %s installation image."),
> +                             getProductName());
>
>             newtWinMessage(_("Error"), _("OK"), buf);
>             free(buf);
> @@ -331,12 +293,9 @@ char * mountNfsImage(struct installMethod * method,
>         case NFS_STAGE_UPDATES: {
>             char *buf;
>
> -            if (asprintf(&buf, "%.*s/RHupdates",
> -                         (int) (strrchr(fullPath, '/')-fullPath),
> -                         fullPath) == -1) {
> -                logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                abort();
> -            }
> +            CHECKED_ASPRINTF(&buf, "%.*s/RHupdates",
> +                             (int) (strrchr(fullPath, '/')-fullPath),
> +                             fullPath);
>
>             logMessage(INFO, "mounting nfs path %s for updates", buf);
>
> @@ -402,10 +361,7 @@ void setKickstartNfs(struct loaderData_s * loaderData, int argc,
>
>     substr = strstr(dir, ".img");
>     if (!substr || (substr && *(substr+4) != '\0')) {
> -        if (asprintf(&(loaderData->instRepo), "nfs:%s:%s", host, dir) == -1) {
> -            logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -            abort();
> -        }
> +        CHECKED_ASPRINTF(&(loaderData->instRepo), "nfs:%s:%s", host, dir);
>
>         logMessage(INFO, "results of nfs, host is %s, dir is %s, opts are '%s'",
>                    host, dir, mountOpts);
> @@ -503,18 +459,10 @@ int getFileFromNfs(char * url, char * dest, struct loaderData_s * loaderData) {
>
>             filename = nm_dhcp4_config_get_one_option(dhcp, "filename");
>             if (filename == NULL) {
> -                 if (asprintf(&url, "%s:/kickstart/", nextserver) == -1) {
> -                     logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                     abort();
> -                 }
> -
> +                CHECKED_ASPRINTF(&url, "%s:/kickstart/", nextserver);
>                 logMessage(ERROR, "bootp: no bootfile received");
>             } else {
> -                 if (asprintf(&url, "%s:%s", nextserver, filename) == -1) {
> -                     logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                     abort();
> -                 }
> -
> +                CHECKED_ASPRINTF(&url, "%s:%s", nextserver, filename);
>                 logMessage(INFO, "bootp: bootfile is %s", filename);
>             }
>
> @@ -545,15 +493,9 @@ int getFileFromNfs(char * url, char * dest, struct loaderData_s * loaderData) {
>         chk = host + strlen(host)-1;
>
>         if (*chk == '/' || *path == '/') {
> -            if (asprintf(&host, "%s:%s", host, path) == -1) {
> -                logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                abort();
> -            }
> +            CHECKED_ASPRINTF(&host, "%s:%s", host, path);
>         } else {
> -            if (asprintf(&host, "%s:/%s", host, path) == -1) {
> -                logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -                abort();
> -            }
> +            CHECKED_ASPRINTF(&host, "%s:/%s", host, path);
>         }
>     }
>
> @@ -562,10 +504,7 @@ int getFileFromNfs(char * url, char * dest, struct loaderData_s * loaderData) {
>     if (!doPwMount(host, "/tmp/mnt", "nfs", opts, NULL)) {
>         char * buf;
>
> -        if (asprintf(&buf, "/tmp/mnt/%s", file) == -1) {
> -            logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
> -            abort();
> -        }
> +        CHECKED_ASPRINTF(&buf, "/tmp/mnt/%s", file);
>
>         if (copyFile(buf, dest)) {
>             logMessage(ERROR, "failed to copy file to %s", dest);

- -- 
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkr9t8YACgkQ5hsjjIy1VklSegCfTwH19eoVl/uxhSFHuae1zHnO
HE4An3e1PVAX4jqLAmWDUiMFhtuPrKQN
=rMB2
-----END PGP SIGNATURE-----

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