From 042fb88fea1ad4e5de18f1ac1be4b4286feff7aa Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 17 Mar 2014 13:24:09 -0400 Subject: [PATCH 1/7] ifcfg-rh: remove ifcfg inheritance code Nothing was using it, so simplify things by getting rid of it. --- src/settings/plugins/ifcfg-rh/shvar.c | 76 +++++---------------------- src/settings/plugins/ifcfg-rh/shvar.h | 1 - 2 files changed, 14 insertions(+), 63 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index cf03c6dab4..b57a8fbfa2 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -4,12 +4,6 @@ * Implementation of non-destructively reading/writing files containing * only shell variable declarations and full-line comments. * - * Includes explicit inheritance mechanism intended for use with - * Red Hat Linux ifcfg-* files. There is no protection against - * inheritance loops; they will generally cause stack overflows. - * Furthermore, they are only intended for one level of inheritance; - * the value setting algorithm assumes this. - * * Copyright 1999,2000 Red Hat, Inc. * * This is free software; you can redistribute it and/or modify it @@ -241,16 +235,12 @@ svGetValue(shvarFile *s, const char *key, gboolean verbatim) } g_free(keyString); - if (value) { - if (value[0]) { - return value; - } else { - g_free(value); - return NULL; - } + if (value && value[0]) { + return value; + } else { + g_free(value); + return NULL; } - if (s->parent) value = svGetValue(s->parent, key, verbatim); - return value; } /* return 1 if resolves to any truth value (e.g. "yes", "y", "true") @@ -283,31 +273,13 @@ svTrueValue(shvarFile *s, const char *key, int def) /* Set the variable equal to the value . * If does not exist, and the pointer is set, append - * the key=value pair after that line. Otherwise, prepend the pair - * to the top of the file. Here's the algorithm, as the C code - * seems to be rather dense: - * - * if (value == NULL), then: - * if val2 (parent): change line to key= or append line key= - * if val1 (this) : delete line - * else noop - * else use this table: - * val2 - * NULL value other - * v NULL append line noop append line - * a - * l value noop noop noop - * 1 - * other change line delete line change line - * - * No changes are ever made to the parent config file, only to the - * specific file passed on the command line. - * + * the key=value pair after that line. Otherwise, append the pair + * to the bottom of the file. */ void svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim) { - char *newval = NULL, *val1 = NULL, *val2 = NULL; + char *newval = NULL, *oldval = NULL; char *keyValue; g_assert(s); @@ -318,19 +290,11 @@ svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim) newval = verbatim ? g_strdup(value) : svEscape(value); keyValue = g_strdup_printf("%s=%s", key, newval ? newval : ""); - val1 = svGetValue(s, key, FALSE); - if (val1 && newval && !strcmp(val1, newval)) goto bail; - if (s->parent) val2 = svGetValue(s->parent, key, FALSE); + oldval = svGetValue(s, key, FALSE); if (!newval || !newval[0]) { - /* delete value somehow */ - if (val2) { - /* change/append line to get key= */ - if (s->current) s->current->data = keyValue; - else s->lineList = g_list_append(s->lineList, keyValue); - s->modified = 1; - goto end; - } else if (val1) { + /* delete value */ + if (oldval) { /* delete line */ s->lineList = g_list_remove_link(s->lineList, s->current); g_list_free_1(s->current); @@ -339,25 +303,14 @@ svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim) goto bail; /* do not need keyValue */ } - if (!val1) { - if (val2 && !strcmp(val2, newval)) goto end; + if (!oldval) { /* append line */ s->lineList = g_list_append(s->lineList, keyValue); s->modified = 1; goto end; } - /* deal with a whole line of noops */ - if (val1 && !strcmp(val1, newval)) goto end; - - /* At this point, val1 && val1 != value */ - if (val2 && !strcmp(val2, newval)) { - /* delete line */ - s->lineList = g_list_remove_link(s->lineList, s->current); - g_list_free_1(s->current); - s->modified = 1; - goto bail; /* do not need keyValue */ - } else { + if (strcmp(oldval, newval) != 0) { /* change line */ if (s->current) s->current->data = keyValue; else s->lineList = g_list_append(s->lineList, keyValue); @@ -366,8 +319,7 @@ svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim) end: g_free(newval); - g_free(val1); - g_free(val2); + g_free(oldval); return; bail: diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h index 4b650d2a9a..4ccf83bb81 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.h +++ b/src/settings/plugins/ifcfg-rh/shvar.h @@ -44,7 +44,6 @@ struct _shvarFile { GList *lineList; /* read-only */ GList *current; /* set implicitly or explicitly, points to element of lineList */ - shvarFile *parent; /* set explicitly */ int modified; /* ignore */ }; From b1049940781556a3b8ca7230ae4c363dda09130e Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 20 Mar 2014 10:32:48 -0400 Subject: [PATCH 2/7] ifcfg-rh: use g_ascii_strcasecmp() in svTrueValue() strcasecmp() is locale-dependent, which is not what we want --- src/settings/plugins/ifcfg-rh/shvar.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index b57a8fbfa2..c6c6808e45 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -256,15 +256,15 @@ svTrueValue(shvarFile *s, const char *key, int def) tmp = svGetValue(s, key, FALSE); if (!tmp) return returnValue; - if ( (!strcasecmp("yes", tmp)) || - (!strcasecmp("true", tmp)) || - (!strcasecmp("t", tmp)) || - (!strcasecmp("y", tmp)) ) returnValue = 1; + if ( (!g_ascii_strcasecmp("yes", tmp)) || + (!g_ascii_strcasecmp("true", tmp)) || + (!g_ascii_strcasecmp("t", tmp)) || + (!g_ascii_strcasecmp("y", tmp)) ) returnValue = 1; else - if ( (!strcasecmp("no", tmp)) || - (!strcasecmp("false", tmp)) || - (!strcasecmp("f", tmp)) || - (!strcasecmp("n", tmp)) ) returnValue = 0; + if ( (!g_ascii_strcasecmp("no", tmp)) || + (!g_ascii_strcasecmp("false", tmp)) || + (!g_ascii_strcasecmp("f", tmp)) || + (!g_ascii_strcasecmp("n", tmp)) ) returnValue = 0; g_free (tmp); return returnValue; From 5bb45373d483fc70fca59ff4244c48722ce75141 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 17 Mar 2014 13:32:18 -0400 Subject: [PATCH 3/7] ifcfg-rh: fix up lowlevel ifcfg file reading code shvar.c was assuming it could do a single read() to read in the ifcfg file, without taking partial reads or EINTR into account. Fix that. Also, it was keeping the raw contents of the ifcfg file in the shvarFile even though it never looked at it after svOpenFile(). (Presumably lineList originally consisted of pointers into arena, but that had to be changed to support readwrite.) Fix that. It would simplify things further to use g_file_get_contents() and g_file_set_contents(), but the current code is perhaps more resilient to symlink attacks because it keeps the fd open? --- src/settings/plugins/ifcfg-rh/shvar.c | 21 +++++++++++++++------ src/settings/plugins/ifcfg-rh/shvar.h | 1 - 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index c6c6808e45..db1ed4da37 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -22,6 +22,7 @@ * */ +#include #include #include #include @@ -56,17 +57,27 @@ svOpenFile(const char *name, gboolean create) if (s->fd != -1) { struct stat buf; - char *p, *q; + char *arena, *p, *q; + ssize_t nread, total = 0; if (fstat(s->fd, &buf) < 0) goto bail; - s->arena = g_malloc0(buf.st_size + 1); + arena = g_malloc(buf.st_size + 1); + arena[buf.st_size] = '\0'; - if (read(s->fd, s->arena, buf.st_size) < 0) goto bail; + while (total < buf.st_size) { + nread = read(s->fd, arena + total, buf.st_size - total); + if (nread == -1 && errno == EINTR) + continue; + if (nread <= 0) + goto bail; + total += nread; + } /* we'd use g_strsplit() here, but we want a list, not an array */ - for(p = s->arena; (q = strchr(p, '\n')) != NULL; p = q + 1) { + for(p = arena; (q = strchr(p, '\n')) != NULL; p = q + 1) { s->lineList = g_list_append(s->lineList, g_strndup(p, q - p)); } + g_free (arena); /* closefd is set if we opened the file read-only, so go ahead and close it, because we can't write to it anyway */ @@ -84,7 +95,6 @@ svOpenFile(const char *name, gboolean create) bail: if (s->fd != -1) close(s->fd); - g_free (s->arena); g_free (s->fileName); g_free (s); return NULL; @@ -374,7 +384,6 @@ svCloseFile(shvarFile *s) if (s->fd != -1) close(s->fd); - g_free(s->arena); g_free(s->fileName); g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */ g_free(s); diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h index 4ccf83bb81..b55cf06b39 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.h +++ b/src/settings/plugins/ifcfg-rh/shvar.h @@ -40,7 +40,6 @@ typedef struct _shvarFile shvarFile; struct _shvarFile { char *fileName; /* read-only */ int fd; /* read-only */ - char *arena; /* ignore */ GList *lineList; /* read-only */ GList *current; /* set implicitly or explicitly, points to element of lineList */ From 110cb0641431b4e622e7a32a29aea4164a80d01b Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 17 Mar 2014 13:00:21 -0400 Subject: [PATCH 4/7] ifcfg-rh: (trivial) syntactic code style fixes to shvar.[ch] --- src/settings/plugins/ifcfg-rh/shvar.c | 406 +++++++++++++------------- src/settings/plugins/ifcfg-rh/shvar.h | 47 ++- 2 files changed, 228 insertions(+), 225 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index db1ed4da37..efc02e7726 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -1,3 +1,4 @@ +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */ /* * shvar.c * @@ -34,85 +35,89 @@ #include "shvar.h" /* Open the file , returning a shvarFile on success and NULL on failure. - Add a wrinkle to let the caller specify whether or not to create the file - (actually, return a structure anyway) if it doesn't exist. */ + * Add a wrinkle to let the caller specify whether or not to create the file + * (actually, return a structure anyway) if it doesn't exist. + */ static shvarFile * -svOpenFile(const char *name, gboolean create) +svOpenFile (const char *name, gboolean create) { - shvarFile *s = NULL; - int closefd = 0; + shvarFile *s = NULL; + int closefd = 0; - s = g_malloc0(sizeof(shvarFile)); + s = g_malloc0 (sizeof (shvarFile)); - s->fd = -1; - if (create) - s->fd = open(name, O_RDWR); /* NOT O_CREAT */ - - if (!create || s->fd == -1) { - /* try read-only */ - s->fd = open(name, O_RDONLY); /* NOT O_CREAT */ - if (s->fd != -1) closefd = 1; - } - s->fileName = g_strdup(name); + s->fd = -1; + if (create) + s->fd = open (name, O_RDWR); /* NOT O_CREAT */ - if (s->fd != -1) { - struct stat buf; - char *arena, *p, *q; - ssize_t nread, total = 0; + if (!create || s->fd == -1) { + /* try read-only */ + s->fd = open (name, O_RDONLY); /* NOT O_CREAT */ + if (s->fd != -1) + closefd = 1; + } + s->fileName = g_strdup (name); - if (fstat(s->fd, &buf) < 0) goto bail; - arena = g_malloc(buf.st_size + 1); - arena[buf.st_size] = '\0'; + if (s->fd != -1) { + struct stat buf; + char *arena, *p, *q; + ssize_t nread, total = 0; - while (total < buf.st_size) { - nread = read(s->fd, arena + total, buf.st_size - total); - if (nread == -1 && errno == EINTR) - continue; - if (nread <= 0) + if (fstat (s->fd, &buf) < 0) goto bail; - total += nread; + arena = g_malloc (buf.st_size + 1); + arena[buf.st_size] = '\0'; + + while (total < buf.st_size) { + nread = read (s->fd, arena + total, buf.st_size - total); + if (nread == -1 && errno == EINTR) + continue; + if (nread <= 0) + goto bail; + total += nread; + } + + /* we'd use g_strsplit() here, but we want a list, not an array */ + for (p = arena; (q = strchr (p, '\n')) != NULL; p = q + 1) + s->lineList = g_list_append (s->lineList, g_strndup (p, q - p)); + g_free (arena); + + /* closefd is set if we opened the file read-only, so go ahead and + * close it, because we can't write to it anyway + */ + if (closefd) { + close (s->fd); + s->fd = -1; + } + + return s; } - /* we'd use g_strsplit() here, but we want a list, not an array */ - for(p = arena; (q = strchr(p, '\n')) != NULL; p = q + 1) { - s->lineList = g_list_append(s->lineList, g_strndup(p, q - p)); - } - g_free (arena); + if (create) + return s; - /* closefd is set if we opened the file read-only, so go ahead and - close it, because we can't write to it anyway */ - if (closefd) { - close(s->fd); - s->fd = -1; - } - - return s; - } - - if (create) { - return s; - } - -bail: - if (s->fd != -1) close(s->fd); - g_free (s->fileName); - g_free (s); - return NULL; + bail: + if (s->fd != -1) + close (s->fd); + g_free (s->fileName); + g_free (s); + return NULL; } /* Open the file , return shvarFile on success, NULL on failure */ shvarFile * -svNewFile(const char *name) +svNewFile (const char *name) { - return svOpenFile(name, FALSE); + return svOpenFile (name, FALSE); } /* Create a new file structure, returning actual data if the file exists, - * and a suitable starting point if it doesn't. */ + * and a suitable starting point if it doesn't. + */ shvarFile * -svCreateFile(const char *name) +svCreateFile (const char *name) { - return svOpenFile(name, TRUE); + return svOpenFile (name, TRUE); } /* remove escaped characters in place */ @@ -122,7 +127,7 @@ svUnescape (char *s) size_t len, idx_rd = 0, idx_wr = 0; char c; - len = strlen(s); + len = strlen (s); if (len < 2) { if (s[0] == '\\') s[0] = '\0'; @@ -159,7 +164,8 @@ svUnescape (char *s) } /* idx_rd points to the first escape. Walk the string and shift the - * characters from idx_rd to idx_wr. */ + * characters from idx_rd to idx_wr. + */ while ((c = s[idx_rd++])) { if (c == '\\') { if (s[idx_rd] == '\0') { @@ -180,38 +186,44 @@ svUnescape (char *s) static const char escapees[] = "\"'\\$~`"; /* must be escaped */ static const char spaces[] = " \t|&;()<>"; /* only require "" */ static const char newlines[] = "\n\r"; /* will be removed */ + char * -svEscape(const char *s) { - char *new; - int i, j, mangle = 0, space = 0, newline = 0; - int newlen, slen; +svEscape (const char *s) +{ + char *new; + int i, j, mangle = 0, space = 0, newline = 0; + int newlen, slen; - slen = strlen(s); + slen = strlen (s); - for (i = 0; i < slen; i++) { - if (strchr(escapees, s[i])) mangle++; - if (strchr(spaces, s[i])) space++; - if (strchr(newlines, s[i])) newline++; - } - if (!mangle && !space && !newline) return strdup(s); - - newlen = slen + mangle - newline + 3; /* 3 is extra ""\0 */ - new = g_malloc0(newlen); - - j = 0; - new[j++] = '"'; - for (i = 0; i < slen; i++) { - if (strchr(newlines, s[i])) - continue; - if (strchr(escapees, s[i])) { - new[j++] = '\\'; + for (i = 0; i < slen; i++) { + if (strchr (escapees, s[i])) + mangle++; + if (strchr (spaces, s[i])) + space++; + if (strchr (newlines, s[i])) + newline++; } - new[j++] = s[i]; - } - new[j++] = '"'; - g_assert(j == slen + mangle - newline + 2); /* j is the index of the '\0' */ + if (!mangle && !space && !newline) + return strdup (s); - return new; + newlen = slen + mangle - newline + 3; /* 3 is extra ""\0 */ + new = g_malloc0 (newlen); + + j = 0; + new[j++] = '"'; + for (i = 0; i < slen; i++) { + if (strchr (newlines, s[i])) + continue; + if (strchr (escapees, s[i])) { + new[j++] = '\\'; + } + new[j++] = s[i]; + } + new[j++] = '"'; + g_assert (j == slen + mangle - newline + 2); /* j is the index of the '\0' */ + + return new; } /* Get the value associated with the key, and leave the current pointer @@ -219,38 +231,38 @@ svEscape(const char *s) { * be freed by the caller. */ char * -svGetValue(shvarFile *s, const char *key, gboolean verbatim) +svGetValue (shvarFile *s, const char *key, gboolean verbatim) { - char *value = NULL; - char *line; - char *keyString; - int len; + char *value = NULL; + char *line; + char *keyString; + int len; - g_assert(s); - g_assert(key); + g_assert (s); + g_assert (key); - keyString = g_malloc0(strlen(key) + 2); - strcpy(keyString, key); - keyString[strlen(key)] = '='; - len = strlen(keyString); + keyString = g_malloc0 (strlen (key) + 2); + strcpy (keyString, key); + keyString[strlen (key)] = '='; + len = strlen (keyString); - for (s->current = s->lineList; s->current; s->current = s->current->next) { - line = s->current->data; - if (!strncmp(keyString, line, len)) { - value = g_strdup(line + len); - if (!verbatim) - svUnescape(value); - break; + for (s->current = s->lineList; s->current; s->current = s->current->next) { + line = s->current->data; + if (!strncmp (keyString, line, len)) { + value = g_strdup (line + len); + if (!verbatim) + svUnescape (value); + break; + } } - } - g_free(keyString); + g_free (keyString); - if (value && value[0]) { - return value; - } else { - g_free(value); - return NULL; - } + if (value && value[0]) { + return value; + } else { + g_free (value); + return NULL; + } } /* return 1 if resolves to any truth value (e.g. "yes", "y", "true") @@ -258,26 +270,28 @@ svGetValue(shvarFile *s, const char *key, gboolean verbatim) * return otherwise */ int -svTrueValue(shvarFile *s, const char *key, int def) +svTrueValue (shvarFile *s, const char *key, int def) { - char *tmp; - int returnValue = def; + char *tmp; + int returnValue = def; - tmp = svGetValue(s, key, FALSE); - if (!tmp) return returnValue; + tmp = svGetValue (s, key, FALSE); + if (!tmp) + return returnValue; - if ( (!g_ascii_strcasecmp("yes", tmp)) || - (!g_ascii_strcasecmp("true", tmp)) || - (!g_ascii_strcasecmp("t", tmp)) || - (!g_ascii_strcasecmp("y", tmp)) ) returnValue = 1; - else - if ( (!g_ascii_strcasecmp("no", tmp)) || - (!g_ascii_strcasecmp("false", tmp)) || - (!g_ascii_strcasecmp("f", tmp)) || - (!g_ascii_strcasecmp("n", tmp)) ) returnValue = 0; + if ( !g_ascii_strcasecmp ("yes", tmp) + || !g_ascii_strcasecmp ("true", tmp) + || !g_ascii_strcasecmp ("t", tmp) + || !g_ascii_strcasecmp ("y", tmp)) + returnValue = 1; + else if ( !g_ascii_strcasecmp ("no", tmp) + || !g_ascii_strcasecmp ("false", tmp) + || !g_ascii_strcasecmp ("f", tmp) + || !g_ascii_strcasecmp ("n", tmp)) + returnValue = 0; - g_free (tmp); - return returnValue; + g_free (tmp); + return returnValue; } @@ -287,54 +301,56 @@ svTrueValue(shvarFile *s, const char *key, int def) * to the bottom of the file. */ void -svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim) +svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim) { - char *newval = NULL, *oldval = NULL; - char *keyValue; + char *newval = NULL, *oldval = NULL; + char *keyValue; - g_assert(s); - g_assert(key); - /* value may be NULL */ + g_assert(s); + g_assert(key); + /* value may be NULL */ - if (value) - newval = verbatim ? g_strdup(value) : svEscape(value); - keyValue = g_strdup_printf("%s=%s", key, newval ? newval : ""); + if (value) + newval = verbatim ? g_strdup (value) : svEscape (value); + keyValue = g_strdup_printf ("%s=%s", key, newval ? newval : ""); - oldval = svGetValue(s, key, FALSE); + oldval = svGetValue (s, key, FALSE); - if (!newval || !newval[0]) { - /* delete value */ - if (oldval) { - /* delete line */ - s->lineList = g_list_remove_link(s->lineList, s->current); - g_list_free_1(s->current); - s->modified = 1; + if (!newval || !newval[0]) { + /* delete value */ + if (oldval) { + /* delete line */ + s->lineList = g_list_remove_link (s->lineList, s->current); + g_list_free_1 (s->current); + s->modified = 1; + } + goto bail; /* do not need keyValue */ } - goto bail; /* do not need keyValue */ - } - if (!oldval) { - /* append line */ - s->lineList = g_list_append(s->lineList, keyValue); - s->modified = 1; + if (!oldval) { + /* append line */ + s->lineList = g_list_append (s->lineList, keyValue); + s->modified = 1; + goto end; + } + + if (strcmp (oldval, newval) != 0) { + /* change line */ + if (s->current) + s->current->data = keyValue; + else + s->lineList = g_list_append (s->lineList, keyValue); + s->modified = 1; + } + + end: + g_free (newval); + g_free (oldval); + return; + + bail: + g_free (keyValue); goto end; - } - - if (strcmp(oldval, newval) != 0) { - /* change line */ - if (s->current) s->current->data = keyValue; - else s->lineList = g_list_append(s->lineList, keyValue); - s->modified = 1; - } - -end: - g_free(newval); - g_free(oldval); - return; - -bail: - g_free (keyValue); - goto end; } /* Write the current contents iff modified. Returns -1 on error @@ -344,45 +360,45 @@ bail: * open() syscall. */ int -svWriteFile(shvarFile *s, int mode) +svWriteFile (shvarFile *s, int mode) { - FILE *f; - int tmpfd; + FILE *f; + int tmpfd; - if (s->modified) { - if (s->fd == -1) - s->fd = open(s->fileName, O_WRONLY|O_CREAT, mode); - if (s->fd == -1) - return -1; - if (ftruncate(s->fd, 0) < 0) - return -1; + if (s->modified) { + if (s->fd == -1) + s->fd = open (s->fileName, O_WRONLY | O_CREAT, mode); + if (s->fd == -1) + return -1; + if (ftruncate (s->fd, 0) < 0) + return -1; - tmpfd = dup(s->fd); - if (tmpfd == -1) - return -1; - f = fdopen(tmpfd, "w"); - fseek(f, 0, SEEK_SET); - for (s->current = s->lineList; s->current; s->current = s->current->next) { - char *line = s->current->data; - fprintf(f, "%s\n", line); + tmpfd = dup (s->fd); + if (tmpfd == -1) + return -1; + f = fdopen (tmpfd, "w"); + fseek (f, 0, SEEK_SET); + for (s->current = s->lineList; s->current; s->current = s->current->next) { + char *line = s->current->data; + fprintf (f, "%s\n", line); + } + fclose (f); } - fclose(f); - } - return 0; + return 0; } - + /* Close the file descriptor (if open) and delete the shvarFile. * Returns -1 on error and 0 on success. */ int -svCloseFile(shvarFile *s) +svCloseFile (shvarFile *s) { + g_assert (s); - g_assert(s); - - if (s->fd != -1) close(s->fd); + if (s->fd != -1) + close (s->fd); g_free(s->fileName); g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */ diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h index b55cf06b39..609ab61370 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.h +++ b/src/settings/plugins/ifcfg-rh/shvar.h @@ -1,3 +1,4 @@ +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */ /* * shvar.h * @@ -32,50 +33,42 @@ #include -#ifdef __cplusplus -extern "C" { -#endif /* __cplusplus */ +G_BEGIN_DECLS typedef struct _shvarFile shvarFile; struct _shvarFile { - char *fileName; /* read-only */ - int fd; /* read-only */ - GList *lineList; /* read-only */ - GList *current; /* set implicitly or explicitly, - points to element of lineList */ - int modified; /* ignore */ + char *fileName; /* read-only */ + int fd; /* read-only */ + GList *lineList; /* read-only */ + GList *current; /* set implicitly or explicitly, points to element of lineList */ + int modified; /* ignore */ }; /* Create the file , return shvarFile on success, NULL on failure */ -shvarFile * -svCreateFile(const char *name); +shvarFile *svCreateFile (const char *name); /* Open the file , return shvarFile on success, NULL on failure */ -shvarFile * -svNewFile(const char *name); +shvarFile *svNewFile (const char *name); /* Get the value associated with the key, and leave the current pointer * pointing at the line containing the value. The char* returned MUST * be freed by the caller. */ -char * -svGetValue(shvarFile *s, const char *key, gboolean verbatim); +char *svGetValue (shvarFile *s, const char *key, gboolean verbatim); /* return 1 if resolves to any truth value (e.g. "yes", "y", "true") * return 0 if resolves to any non-truth value (e.g. "no", "n", "false") * return otherwise */ -int -svTrueValue(shvarFile *s, const char *key, int def); +int svTrueValue (shvarFile *s, const char *key, int def); /* Set the variable equal to the value . * If does not exist, and the pointer is set, append * the key=value pair after that line. Otherwise, prepend the pair * to the top of the file. */ -void -svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim); +void svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim); /* Write the current contents iff modified. Returns -1 on error @@ -84,25 +77,19 @@ svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim); * re-writing an existing file, and is passed unchanged to the * open() syscall. */ -int -svWriteFile(shvarFile *s, int mode); +int svWriteFile (shvarFile *s, int mode); /* Close the file descriptor (if open) and delete the shvarFile. * Returns -1 on error and 0 on success. */ -int -svCloseFile(shvarFile *s); +int svCloseFile (shvarFile *s); /* Return a new escaped string */ -char * -svEscape(const char *s); +char *svEscape (const char *s); /* Unescape a string in-place */ -void -svUnescape(char *s); +void svUnescape (char *s); -#ifdef __cplusplus -} -#endif /* __cplusplus */ +G_END_DECLS #endif /* ! _SHVAR_H */ From 5b4cc33cbd685f6f1e26193ab055b37e28a09fb9 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 17 Mar 2014 13:00:21 -0400 Subject: [PATCH 5/7] ifcfg-rh: semantic code style fixes to shvar.[ch] --- src/settings/plugins/ifcfg-rh/shvar.c | 77 +++++++++++++------------- src/settings/plugins/ifcfg-rh/shvar.h | 20 +++---- src/settings/plugins/ifcfg-rh/writer.c | 6 +- 3 files changed, 51 insertions(+), 52 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index efc02e7726..3af20eda89 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -42,9 +42,9 @@ static shvarFile * svOpenFile (const char *name, gboolean create) { shvarFile *s = NULL; - int closefd = 0; + gboolean closefd = FALSE; - s = g_malloc0 (sizeof (shvarFile)); + s = g_slice_new0 (shvarFile); s->fd = -1; if (create) @@ -54,7 +54,7 @@ svOpenFile (const char *name, gboolean create) /* try read-only */ s->fd = open (name, O_RDONLY); /* NOT O_CREAT */ if (s->fd != -1) - closefd = 1; + closefd = TRUE; } s->fileName = g_strdup (name); @@ -100,7 +100,7 @@ svOpenFile (const char *name, gboolean create) if (s->fd != -1) close (s->fd); g_free (s->fileName); - g_free (s); + g_slice_free (shvarFile, s); return NULL; } @@ -208,7 +208,7 @@ svEscape (const char *s) return strdup (s); newlen = slen + mangle - newline + 3; /* 3 is extra ""\0 */ - new = g_malloc0 (newlen); + new = g_malloc (newlen); j = 0; new[j++] = '"'; @@ -221,7 +221,9 @@ svEscape (const char *s) new[j++] = s[i]; } new[j++] = '"'; - g_assert (j == slen + mangle - newline + 2); /* j is the index of the '\0' */ + new[j++] = '\0' +; + g_assert (j == slen + mangle - newline + 3); return new; } @@ -238,12 +240,10 @@ svGetValue (shvarFile *s, const char *key, gboolean verbatim) char *keyString; int len; - g_assert (s); - g_assert (key); + g_return_val_if_fail (s != NULL, NULL); + g_return_val_if_fail (key != NULL, NULL); - keyString = g_malloc0 (strlen (key) + 2); - strcpy (keyString, key); - keyString[strlen (key)] = '='; + keyString = g_strdup_printf ("%s=", key); len = strlen (keyString); for (s->current = s->lineList; s->current; s->current = s->current->next) { @@ -265,15 +265,15 @@ svGetValue (shvarFile *s, const char *key, gboolean verbatim) } } -/* return 1 if resolves to any truth value (e.g. "yes", "y", "true") - * return 0 if resolves to any non-truth value (e.g. "no", "n", "false") +/* return TRUE if resolves to any truth value (e.g. "yes", "y", "true") + * return FALSE if resolves to any non-truth value (e.g. "no", "n", "false") * return otherwise */ -int -svTrueValue (shvarFile *s, const char *key, int def) +gboolean +svTrueValue (shvarFile *s, const char *key, gboolean def) { char *tmp; - int returnValue = def; + gboolean returnValue = def; tmp = svGetValue (s, key, FALSE); if (!tmp) @@ -283,12 +283,12 @@ svTrueValue (shvarFile *s, const char *key, int def) || !g_ascii_strcasecmp ("true", tmp) || !g_ascii_strcasecmp ("t", tmp) || !g_ascii_strcasecmp ("y", tmp)) - returnValue = 1; + returnValue = TRUE; else if ( !g_ascii_strcasecmp ("no", tmp) || !g_ascii_strcasecmp ("false", tmp) || !g_ascii_strcasecmp ("f", tmp) || !g_ascii_strcasecmp ("n", tmp)) - returnValue = 0; + returnValue = FALSE; g_free (tmp); return returnValue; @@ -306,8 +306,8 @@ svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim) char *newval = NULL, *oldval = NULL; char *keyValue; - g_assert(s); - g_assert(key); + g_return_if_fail (s != NULL); + g_return_if_fail (key != NULL); /* value may be NULL */ if (value) @@ -322,7 +322,7 @@ svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim) /* delete line */ s->lineList = g_list_remove_link (s->lineList, s->current); g_list_free_1 (s->current); - s->modified = 1; + s->modified = TRUE; } goto bail; /* do not need keyValue */ } @@ -330,7 +330,7 @@ svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim) if (!oldval) { /* append line */ s->lineList = g_list_append (s->lineList, keyValue); - s->modified = 1; + s->modified = TRUE; goto end; } @@ -340,7 +340,7 @@ svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim) s->current->data = keyValue; else s->lineList = g_list_append (s->lineList, keyValue); - s->modified = 1; + s->modified = TRUE; } end: @@ -353,13 +353,13 @@ svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim) goto end; } -/* Write the current contents iff modified. Returns -1 on error - * and 0 on success. Do not write if no values have been modified. +/* Write the current contents iff modified. Returns FALSE on error + * and TRUE on success. Do not write if no values have been modified. * The mode argument is only used if creating the file, not if * re-writing an existing file, and is passed unchanged to the * open() syscall. */ -int +gboolean svWriteFile (shvarFile *s, int mode) { FILE *f; @@ -369,13 +369,13 @@ svWriteFile (shvarFile *s, int mode) if (s->fd == -1) s->fd = open (s->fileName, O_WRONLY | O_CREAT, mode); if (s->fd == -1) - return -1; + return FALSE; if (ftruncate (s->fd, 0) < 0) - return -1; + return FALSE; tmpfd = dup (s->fd); if (tmpfd == -1) - return -1; + return FALSE; f = fdopen (tmpfd, "w"); fseek (f, 0, SEEK_SET); for (s->current = s->lineList; s->current; s->current = s->current->next) { @@ -385,23 +385,22 @@ svWriteFile (shvarFile *s, int mode) fclose (f); } - return 0; + return TRUE; } -/* Close the file descriptor (if open) and delete the shvarFile. - * Returns -1 on error and 0 on success. - */ -int +/* Close the file descriptor (if open) and free the shvarFile. */ +gboolean svCloseFile (shvarFile *s) { - g_assert (s); + g_return_val_if_fail (s != NULL, FALSE); if (s->fd != -1) close (s->fd); - g_free(s->fileName); - g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */ - g_free(s); - return 0; + g_free (s->fileName); + g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */ + g_slice_free (shvarFile, s); + + return TRUE; } diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h index 609ab61370..3062a0d5cd 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.h +++ b/src/settings/plugins/ifcfg-rh/shvar.h @@ -41,7 +41,7 @@ struct _shvarFile { int fd; /* read-only */ GList *lineList; /* read-only */ GList *current; /* set implicitly or explicitly, points to element of lineList */ - int modified; /* ignore */ + gboolean modified; /* ignore */ }; @@ -57,11 +57,11 @@ shvarFile *svNewFile (const char *name); */ char *svGetValue (shvarFile *s, const char *key, gboolean verbatim); -/* return 1 if resolves to any truth value (e.g. "yes", "y", "true") - * return 0 if resolves to any non-truth value (e.g. "no", "n", "false") +/* return TRUE if resolves to any truth value (e.g. "yes", "y", "true") + * return FALSE if resolves to any non-truth value (e.g. "no", "n", "false") * return otherwise */ -int svTrueValue (shvarFile *s, const char *key, int def); +gboolean svTrueValue (shvarFile *s, const char *key, gboolean def); /* Set the variable equal to the value . * If does not exist, and the pointer is set, append @@ -71,18 +71,18 @@ int svTrueValue (shvarFile *s, const char *key, int def); void svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim); -/* Write the current contents iff modified. Returns -1 on error - * and 0 on success. Do not write if no values have been modified. +/* Write the current contents iff modified. Returns FALSE on error + * and TRUE on success. Do not write if no values have been modified. * The mode argument is only used if creating the file, not if * re-writing an existing file, and is passed unchanged to the * open() syscall. */ -int svWriteFile (shvarFile *s, int mode); +gboolean svWriteFile (shvarFile *s, int mode); -/* Close the file descriptor (if open) and delete the shvarFile. - * Returns -1 on error and 0 on success. +/* Close the file descriptor (if open) and free the shvarFile. + * Returns FALSE on error and TRUE on success. */ -int svCloseFile (shvarFile *s); +gboolean svCloseFile (shvarFile *s); /* Return a new escaped string */ char *svEscape (const char *s); diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c index 5e962c57cd..ca8b2d79da 100644 --- a/src/settings/plugins/ifcfg-rh/writer.c +++ b/src/settings/plugins/ifcfg-rh/writer.c @@ -118,7 +118,7 @@ set_secret (shvarFile *ifcfg, if (flags == NM_SETTING_SECRET_FLAG_NONE) svSetValue (keyfile, key, value, verbatim); - if (svWriteFile (keyfile, 0600)) { + if (!svWriteFile (keyfile, 0600)) { PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: could not update key file '%s'", keyfile->fileName); svCloseFile (keyfile); @@ -2118,7 +2118,7 @@ write_ip4_setting (NMConnection *connection, shvarFile *ifcfg, GError **error) g_free (gw_key); g_free (metric_key); } - if (svWriteFile (routefile, 0644)) { + if (!svWriteFile (routefile, 0644)) { g_set_error (error, IFCFG_PLUGIN_ERROR, 0, "Could not update route file '%s'", routefile->fileName); svCloseFile (routefile); @@ -2641,7 +2641,7 @@ write_connection (NMConnection *connection, write_connection_setting (s_con, ifcfg); - if (svWriteFile (ifcfg, 0644)) { + if (!svWriteFile (ifcfg, 0644)) { g_set_error (error, IFCFG_PLUGIN_ERROR, 0, "Can't write connection '%s'", ifcfg->fileName); goto out; From 454311c9ecca2fc42029dd5f7c4e65b8bcb193db Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 18 Mar 2014 12:54:16 -0400 Subject: [PATCH 6/7] ifcfg-rh: (trivial) rename svNewFile() to svOpenFile() It's "new" in the sense that it creates a new shvarFile object, but it doesn't create a new file, it just opens an existing one. --- src/settings/plugins/ifcfg-rh/plugin.c | 4 ++-- src/settings/plugins/ifcfg-rh/reader.c | 14 +++++++------- src/settings/plugins/ifcfg-rh/shvar.c | 8 ++++---- src/settings/plugins/ifcfg-rh/shvar.h | 4 ++-- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 16 ++++++++-------- src/settings/plugins/ifcfg-rh/utils.c | 2 +- src/settings/plugins/ifcfg-rh/writer.c | 2 +- 7 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 1f9ed4726d..5fda59da09 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -646,7 +646,7 @@ plugin_get_hostname (SCPluginIfcfg *plugin) return hostname; } - network = svNewFile (SC_NETWORK_FILE); + network = svOpenFile (SC_NETWORK_FILE); if (!network) { PLUGIN_WARN (IFCFG_PLUGIN_NAME, "Could not get hostname: failed to read " SC_NETWORK_FILE); return NULL; @@ -708,7 +708,7 @@ plugin_set_hostname (SCPluginIfcfg *plugin, const char *hostname) g_free (hostname_eol); /* Remove "HOSTNAME" from SC_NETWORK_FILE, if present */ - network = svNewFile (SC_NETWORK_FILE); + network = svOpenFile (SC_NETWORK_FILE); if (network) { svSetValue (network, "HOSTNAME", NULL, FALSE); svWriteFile (network, 0644); diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c index 70e58e9e03..7a0a41649e 100644 --- a/src/settings/plugins/ifcfg-rh/reader.c +++ b/src/settings/plugins/ifcfg-rh/reader.c @@ -682,7 +682,7 @@ read_full_ip4_address (shvarFile *ifcfg, gboolean read_success; /* If no gateway in the ifcfg, try /etc/sysconfig/network instead */ - network_ifcfg = svNewFile (network_file); + network_ifcfg = svOpenFile (network_file); if (network_ifcfg) { read_success = read_ip4_address (network_ifcfg, "GATEWAY", &tmp, error); svCloseFile (network_ifcfg); @@ -1066,7 +1066,7 @@ parse_full_ip6_address (shvarFile *ifcfg, } if (!value) { /* If no gateway in the ifcfg, try global /etc/sysconfig/network instead */ - network_ifcfg = svNewFile (network_file); + network_ifcfg = svOpenFile (network_file); if (network_ifcfg) { value = svGetValue (network_ifcfg, "IPV6_DEFAULTGW", FALSE); svCloseFile (network_ifcfg); @@ -1281,7 +1281,7 @@ make_ip4_setting (shvarFile *ifcfg, never_default = !svTrueValue (ifcfg, "DEFROUTE", TRUE); /* Then check if GATEWAYDEV; it's global and overrides DEFROUTE */ - network_ifcfg = svNewFile (network_file); + network_ifcfg = svOpenFile (network_file); if (network_ifcfg) { char *gatewaydev; @@ -1645,7 +1645,7 @@ make_ip6_setting (shvarFile *ifcfg, * they are global and override IPV6_DEFROUTE * When both are set, the device specified in IPV6_DEFAULTGW takes preference. */ - network_ifcfg = svNewFile (network_file); + network_ifcfg = svOpenFile (network_file); if (network_ifcfg) { char *ipv6_defaultgw, *ipv6_defaultdev; char *default_dev = NULL; @@ -1680,7 +1680,7 @@ make_ip6_setting (shvarFile *ifcfg, str_value = svGetValue (ifcfg, "IPV6INIT", FALSE); ipv6init = svTrueValue (ifcfg, "IPV6INIT", FALSE); if (!str_value) { - network_ifcfg = svNewFile (network_file); + network_ifcfg = svOpenFile (network_file); if (network_ifcfg) { ipv6init = svTrueValue (network_ifcfg, "IPV6INIT", FALSE); svCloseFile (network_ifcfg); @@ -4991,7 +4991,7 @@ uuid_from_file (const char *filename) if (!ifcfg_name) return NULL; - ifcfg = svNewFile (filename); + ifcfg = svOpenFile (filename); if (!ifcfg) return NULL; @@ -5077,7 +5077,7 @@ connection_from_file (const char *filename, return NULL; } - parsed = svNewFile (filename); + parsed = svOpenFile (filename); if (!parsed) { g_set_error (error, IFCFG_PLUGIN_ERROR, 0, "Couldn't parse file '%s'", filename); diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index 3af20eda89..630e9b6fd1 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -39,7 +39,7 @@ * (actually, return a structure anyway) if it doesn't exist. */ static shvarFile * -svOpenFile (const char *name, gboolean create) +svOpenFileInternal (const char *name, gboolean create) { shvarFile *s = NULL; gboolean closefd = FALSE; @@ -106,9 +106,9 @@ svOpenFile (const char *name, gboolean create) /* Open the file , return shvarFile on success, NULL on failure */ shvarFile * -svNewFile (const char *name) +svOpenFile (const char *name) { - return svOpenFile (name, FALSE); + return svOpenFileInternal (name, FALSE); } /* Create a new file structure, returning actual data if the file exists, @@ -117,7 +117,7 @@ svNewFile (const char *name) shvarFile * svCreateFile (const char *name) { - return svOpenFile (name, TRUE); + return svOpenFileInternal (name, TRUE); } /* remove escaped characters in place */ diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h index 3062a0d5cd..d28bc6d7d5 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.h +++ b/src/settings/plugins/ifcfg-rh/shvar.h @@ -45,11 +45,11 @@ struct _shvarFile { }; -/* Create the file , return shvarFile on success, NULL on failure */ +/* Create the file , return a shvarFile (never fails) */ shvarFile *svCreateFile (const char *name); /* Open the file , return shvarFile on success, NULL on failure */ -shvarFile *svNewFile (const char *name); +shvarFile *svOpenFile (const char *name); /* Get the value associated with the key, and leave the current pointer * pointing at the line containing the value. The char* returned MUST diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index f45d788ece..4d774fa967 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -6106,7 +6106,7 @@ test_write_wifi_hidden (void) TEST_SCRATCH_DIR "/network-scripts/", &testfile, &error); - f = svNewFile (testfile); + f = svOpenFile (testfile); g_assert (f); g_assert_no_error (error); @@ -7340,7 +7340,7 @@ test_write_wired_static_ip6_only_gw (gconstpointer user_data) { /* re-read the file to check that what key was written. */ - shvarFile *ifcfg = svNewFile (testfile); + shvarFile *ifcfg = svOpenFile (testfile); g_assert (ifcfg); written_ifcfg_gateway = svGetValue (ifcfg, "IPV6_DEFAULTGW", FALSE); @@ -8382,7 +8382,7 @@ test_write_wifi_open (void) &ignore_error); /* Now make sure that the ESSID item isn't double-quoted (rh #606518) */ - ifcfg = svNewFile (testfile); + ifcfg = svOpenFile (testfile); ASSERT (ifcfg != NULL, "wifi-open-write-reread", "failed to load %s as shvarfile", testfile); @@ -10869,7 +10869,7 @@ test_write_wifi_dynamic_wep_leap (void) * did not get written. Check first that the auth alg is not set to "LEAP" * and next that the only IEEE 802.1x EAP method is "LEAP". */ - ifcfg = svNewFile (testfile); + ifcfg = svOpenFile (testfile); g_assert (ifcfg); tmp = svGetValue (ifcfg, "SECURITYMODE", FALSE); g_assert_cmpstr (tmp, ==, NULL); @@ -11487,7 +11487,7 @@ test_write_wired_ctc_dhcp (void) g_assert (testfile != NULL); /* Ensure the CTCPROT item gets written out as it's own option */ - ifcfg = svNewFile (testfile); + ifcfg = svOpenFile (testfile); g_assert (ifcfg); tmp = svGetValue (ifcfg, "CTCPROT", TRUE); @@ -13864,7 +13864,7 @@ test_write_fcoe_mode (gconstpointer user_data) g_assert (testfile); { - shvarFile *ifcfg = svNewFile (testfile); + shvarFile *ifcfg = svOpenFile (testfile); char *written_mode; g_assert (ifcfg); @@ -13975,7 +13975,7 @@ test_write_team_master (void) TEST_SCRATCH_DIR "/network-scripts/", &testfile, &error); - f = svNewFile (testfile); + f = svOpenFile (testfile); g_assert (f); g_assert_no_error (error); @@ -14092,7 +14092,7 @@ test_write_team_port (void) TEST_SCRATCH_DIR "/network-scripts/", &testfile, &error); - f = svNewFile (testfile); + f = svOpenFile (testfile); g_assert (f); g_assert_no_error (error); diff --git a/src/settings/plugins/ifcfg-rh/utils.c b/src/settings/plugins/ifcfg-rh/utils.c index 5abb4282e5..07efa5cc53 100644 --- a/src/settings/plugins/ifcfg-rh/utils.c +++ b/src/settings/plugins/ifcfg-rh/utils.c @@ -296,7 +296,7 @@ utils_get_extra_ifcfg (const char *parent, const char *tag, gboolean should_crea ifcfg = svCreateFile (path); if (!ifcfg) - ifcfg = svNewFile (path); + ifcfg = svOpenFile (path); g_free (path); return ifcfg; diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c index ca8b2d79da..9c7b3037a3 100644 --- a/src/settings/plugins/ifcfg-rh/writer.c +++ b/src/settings/plugins/ifcfg-rh/writer.c @@ -2529,7 +2529,7 @@ write_connection (NMConnection *connection, if (filename) { /* For existing connections, 'filename' should be full path to ifcfg file */ - ifcfg = svNewFile (filename); + ifcfg = svOpenFile (filename); ifcfg_name = g_strdup (filename); } else { char *escaped; From e43283a288ffe9f00520400a94eef3593ad8d782 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 18 Mar 2014 13:13:12 -0400 Subject: [PATCH 7/7] ifcfg-rh: return proper error messages from svOpenFile() and svWriteFile() --- src/settings/plugins/ifcfg-rh/plugin.c | 6 +- src/settings/plugins/ifcfg-rh/reader.c | 19 +++---- src/settings/plugins/ifcfg-rh/shvar.c | 57 ++++++++++++++----- src/settings/plugins/ifcfg-rh/shvar.h | 10 ++-- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 49 +++++++++------- src/settings/plugins/ifcfg-rh/utils.c | 2 +- src/settings/plugins/ifcfg-rh/writer.c | 27 ++++----- 7 files changed, 96 insertions(+), 74 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 5fda59da09..0bf16758b5 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -646,7 +646,7 @@ plugin_get_hostname (SCPluginIfcfg *plugin) return hostname; } - network = svOpenFile (SC_NETWORK_FILE); + network = svOpenFile (SC_NETWORK_FILE, NULL); if (!network) { PLUGIN_WARN (IFCFG_PLUGIN_NAME, "Could not get hostname: failed to read " SC_NETWORK_FILE); return NULL; @@ -708,10 +708,10 @@ plugin_set_hostname (SCPluginIfcfg *plugin, const char *hostname) g_free (hostname_eol); /* Remove "HOSTNAME" from SC_NETWORK_FILE, if present */ - network = svOpenFile (SC_NETWORK_FILE); + network = svOpenFile (SC_NETWORK_FILE, NULL); if (network) { svSetValue (network, "HOSTNAME", NULL, FALSE); - svWriteFile (network, 0644); + svWriteFile (network, 0644, NULL); svCloseFile (network); } diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c index 7a0a41649e..3b0379c200 100644 --- a/src/settings/plugins/ifcfg-rh/reader.c +++ b/src/settings/plugins/ifcfg-rh/reader.c @@ -682,7 +682,7 @@ read_full_ip4_address (shvarFile *ifcfg, gboolean read_success; /* If no gateway in the ifcfg, try /etc/sysconfig/network instead */ - network_ifcfg = svOpenFile (network_file); + network_ifcfg = svOpenFile (network_file, NULL); if (network_ifcfg) { read_success = read_ip4_address (network_ifcfg, "GATEWAY", &tmp, error); svCloseFile (network_ifcfg); @@ -1066,7 +1066,7 @@ parse_full_ip6_address (shvarFile *ifcfg, } if (!value) { /* If no gateway in the ifcfg, try global /etc/sysconfig/network instead */ - network_ifcfg = svOpenFile (network_file); + network_ifcfg = svOpenFile (network_file, NULL); if (network_ifcfg) { value = svGetValue (network_ifcfg, "IPV6_DEFAULTGW", FALSE); svCloseFile (network_ifcfg); @@ -1281,7 +1281,7 @@ make_ip4_setting (shvarFile *ifcfg, never_default = !svTrueValue (ifcfg, "DEFROUTE", TRUE); /* Then check if GATEWAYDEV; it's global and overrides DEFROUTE */ - network_ifcfg = svOpenFile (network_file); + network_ifcfg = svOpenFile (network_file, NULL); if (network_ifcfg) { char *gatewaydev; @@ -1645,7 +1645,7 @@ make_ip6_setting (shvarFile *ifcfg, * they are global and override IPV6_DEFROUTE * When both are set, the device specified in IPV6_DEFAULTGW takes preference. */ - network_ifcfg = svOpenFile (network_file); + network_ifcfg = svOpenFile (network_file, NULL); if (network_ifcfg) { char *ipv6_defaultgw, *ipv6_defaultdev; char *default_dev = NULL; @@ -1680,7 +1680,7 @@ make_ip6_setting (shvarFile *ifcfg, str_value = svGetValue (ifcfg, "IPV6INIT", FALSE); ipv6init = svTrueValue (ifcfg, "IPV6INIT", FALSE); if (!str_value) { - network_ifcfg = svOpenFile (network_file); + network_ifcfg = svOpenFile (network_file, NULL); if (network_ifcfg) { ipv6init = svTrueValue (network_ifcfg, "IPV6INIT", FALSE); svCloseFile (network_ifcfg); @@ -4991,7 +4991,7 @@ uuid_from_file (const char *filename) if (!ifcfg_name) return NULL; - ifcfg = svOpenFile (filename); + ifcfg = svOpenFile (filename, NULL); if (!ifcfg) return NULL; @@ -5077,12 +5077,9 @@ connection_from_file (const char *filename, return NULL; } - parsed = svOpenFile (filename); - if (!parsed) { - g_set_error (error, IFCFG_PLUGIN_ERROR, 0, - "Couldn't parse file '%s'", filename); + parsed = svOpenFile (filename, error); + if (!parsed) return NULL; - } if (!svTrueValue (parsed, "NM_CONTROLLED", TRUE)) { g_assert (out_unhandled != NULL); diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index 630e9b6fd1..4ba8a631f1 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -39,10 +39,11 @@ * (actually, return a structure anyway) if it doesn't exist. */ static shvarFile * -svOpenFileInternal (const char *name, gboolean create) +svOpenFileInternal (const char *name, gboolean create, GError **error) { shvarFile *s = NULL; gboolean closefd = FALSE; + int errsv; s = g_slice_new0 (shvarFile); @@ -53,7 +54,9 @@ svOpenFileInternal (const char *name, gboolean create) if (!create || s->fd == -1) { /* try read-only */ s->fd = open (name, O_RDONLY); /* NOT O_CREAT */ - if (s->fd != -1) + if (s->fd == -1) + errsv = errno; + else closefd = TRUE; } s->fileName = g_strdup (name); @@ -63,8 +66,10 @@ svOpenFileInternal (const char *name, gboolean create) char *arena, *p, *q; ssize_t nread, total = 0; - if (fstat (s->fd, &buf) < 0) + if (fstat (s->fd, &buf) < 0) { + errsv = errno; goto bail; + } arena = g_malloc (buf.st_size + 1); arena[buf.st_size] = '\0'; @@ -72,8 +77,10 @@ svOpenFileInternal (const char *name, gboolean create) nread = read (s->fd, arena + total, buf.st_size - total); if (nread == -1 && errno == EINTR) continue; - if (nread <= 0) + if (nread <= 0) { + errsv = errno; goto bail; + } total += nread; } @@ -101,14 +108,18 @@ svOpenFileInternal (const char *name, gboolean create) close (s->fd); g_free (s->fileName); g_slice_free (shvarFile, s); + + g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv), + "Could not read file '%s': %s", + name, errsv ? strerror (errsv) : "Unknown error"); return NULL; } /* Open the file , return shvarFile on success, NULL on failure */ shvarFile * -svOpenFile (const char *name) +svOpenFile (const char *name, GError **error) { - return svOpenFileInternal (name, FALSE); + return svOpenFileInternal (name, FALSE, error); } /* Create a new file structure, returning actual data if the file exists, @@ -117,7 +128,7 @@ svOpenFile (const char *name) shvarFile * svCreateFile (const char *name) { - return svOpenFileInternal (name, TRUE); + return svOpenFileInternal (name, TRUE, NULL); } /* remove escaped characters in place */ @@ -360,7 +371,7 @@ svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim) * open() syscall. */ gboolean -svWriteFile (shvarFile *s, int mode) +svWriteFile (shvarFile *s, int mode, GError **error) { FILE *f; int tmpfd; @@ -368,14 +379,32 @@ svWriteFile (shvarFile *s, int mode) if (s->modified) { if (s->fd == -1) s->fd = open (s->fileName, O_WRONLY | O_CREAT, mode); - if (s->fd == -1) + if (s->fd == -1) { + int errsv = errno; + + g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv), + "Could not open file '%s' for writing: %s", + s->fileName, strerror (errsv)); return FALSE; - if (ftruncate (s->fd, 0) < 0) + } + if (ftruncate (s->fd, 0) < 0) { + int errsv = errno; + + g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv), + "Could not overwrite file '%s': %s", + s->fileName, strerror (errsv)); return FALSE; + } tmpfd = dup (s->fd); - if (tmpfd == -1) + if (tmpfd == -1) { + int errsv = errno; + + g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv), + "Internal error writing file '%s': %s", + s->fileName, strerror (errsv)); return FALSE; + } f = fdopen (tmpfd, "w"); fseek (f, 0, SEEK_SET); for (s->current = s->lineList; s->current; s->current = s->current->next) { @@ -390,10 +419,10 @@ svWriteFile (shvarFile *s, int mode) /* Close the file descriptor (if open) and free the shvarFile. */ -gboolean +void svCloseFile (shvarFile *s) { - g_return_val_if_fail (s != NULL, FALSE); + g_return_if_fail (s != NULL); if (s->fd != -1) close (s->fd); @@ -401,6 +430,4 @@ svCloseFile (shvarFile *s) g_free (s->fileName); g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */ g_slice_free (shvarFile, s); - - return TRUE; } diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h index d28bc6d7d5..4cbf1a31a7 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.h +++ b/src/settings/plugins/ifcfg-rh/shvar.h @@ -49,7 +49,7 @@ struct _shvarFile { shvarFile *svCreateFile (const char *name); /* Open the file , return shvarFile on success, NULL on failure */ -shvarFile *svOpenFile (const char *name); +shvarFile *svOpenFile (const char *name, GError **error); /* Get the value associated with the key, and leave the current pointer * pointing at the line containing the value. The char* returned MUST @@ -77,12 +77,10 @@ void svSetValue (shvarFile *s, const char *key, const char *value, gboolean verb * re-writing an existing file, and is passed unchanged to the * open() syscall. */ -gboolean svWriteFile (shvarFile *s, int mode); +gboolean svWriteFile (shvarFile *s, int mode, GError **error); -/* Close the file descriptor (if open) and free the shvarFile. - * Returns FALSE on error and TRUE on success. - */ -gboolean svCloseFile (shvarFile *s); +/* Close the file descriptor (if open) and free the shvarFile. */ +void svCloseFile (shvarFile *s); /* Return a new escaped string */ char *svEscape (const char *s); diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 4d774fa967..082d6fd585 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -6106,12 +6106,13 @@ test_write_wifi_hidden (void) TEST_SCRATCH_DIR "/network-scripts/", &testfile, &error); - f = svOpenFile (testfile); - g_assert (f); - g_assert_no_error (error); g_assert (success); + f = svOpenFile (testfile, &error); + g_assert_no_error (error); + g_assert (f); + /* re-read the file to check that what key was written. */ val = svGetValue (f, "SSID_HIDDEN", FALSE); g_assert (val); @@ -7337,11 +7338,16 @@ test_write_wired_static_ip6_only_gw (gconstpointer user_data) NULL, NULL, &error, &ignore_error); + g_assert_no_error (error); + g_assert (reread); + g_assert (nm_connection_verify (reread, &error)); + g_assert (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); { /* re-read the file to check that what key was written. */ - shvarFile *ifcfg = svOpenFile (testfile); + shvarFile *ifcfg = svOpenFile (testfile, &error); + g_assert_no_error (error); g_assert (ifcfg); written_ifcfg_gateway = svGetValue (ifcfg, "IPV6_DEFAULTGW", FALSE); svCloseFile (ifcfg); @@ -7349,11 +7355,6 @@ test_write_wired_static_ip6_only_gw (gconstpointer user_data) unlink (testfile); - g_assert_no_error (error); - g_assert (reread); - g_assert (nm_connection_verify (reread, &error)); - g_assert (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); - /* access the gateway from the loaded connection. */ s_ip6 = nm_connection_get_setting_ip6_config (reread); g_assert (s_ip6 && nm_setting_ip6_config_get_num_addresses (s_ip6)==1); @@ -8380,11 +8381,12 @@ test_write_wifi_open (void) &route6file, &error, &ignore_error); + g_assert_no_error (error); /* Now make sure that the ESSID item isn't double-quoted (rh #606518) */ - ifcfg = svOpenFile (testfile); - ASSERT (ifcfg != NULL, - "wifi-open-write-reread", "failed to load %s as shvarfile", testfile); + ifcfg = svOpenFile (testfile, &error); + g_assert_no_error (error); + g_assert (ifcfg != NULL); tmp = svGetValue (ifcfg, "ESSID", TRUE); ASSERT (tmp != NULL, @@ -10869,7 +10871,8 @@ test_write_wifi_dynamic_wep_leap (void) * did not get written. Check first that the auth alg is not set to "LEAP" * and next that the only IEEE 802.1x EAP method is "LEAP". */ - ifcfg = svOpenFile (testfile); + ifcfg = svOpenFile (testfile, &error); + g_assert_no_error (error); g_assert (ifcfg); tmp = svGetValue (ifcfg, "SECURITYMODE", FALSE); g_assert_cmpstr (tmp, ==, NULL); @@ -11487,7 +11490,8 @@ test_write_wired_ctc_dhcp (void) g_assert (testfile != NULL); /* Ensure the CTCPROT item gets written out as it's own option */ - ifcfg = svOpenFile (testfile); + ifcfg = svOpenFile (testfile, &error); + g_assert_no_error (error); g_assert (ifcfg); tmp = svGetValue (ifcfg, "CTCPROT", TRUE); @@ -13864,9 +13868,10 @@ test_write_fcoe_mode (gconstpointer user_data) g_assert (testfile); { - shvarFile *ifcfg = svOpenFile (testfile); + shvarFile *ifcfg = svOpenFile (testfile, &error); char *written_mode; + g_assert_no_error (error); g_assert (ifcfg); written_mode = svGetValue (ifcfg, "DCB_APP_FCOE_MODE", FALSE); svCloseFile (ifcfg); @@ -13975,12 +13980,13 @@ test_write_team_master (void) TEST_SCRATCH_DIR "/network-scripts/", &testfile, &error); - f = svOpenFile (testfile); - g_assert (f); - g_assert_no_error (error); g_assert (success); + f = svOpenFile (testfile, &error); + g_assert_no_error (error); + g_assert (f); + /* re-read the file to check that what key was written. */ val = svGetValue (f, "DEVICETYPE", FALSE); g_assert (val); @@ -14092,12 +14098,13 @@ test_write_team_port (void) TEST_SCRATCH_DIR "/network-scripts/", &testfile, &error); - f = svOpenFile (testfile); - g_assert (f); - g_assert_no_error (error); g_assert (success); + f = svOpenFile (testfile, &error); + g_assert_no_error (error); + g_assert (f); + /* re-read the file to check that what key was written. */ val = svGetValue (f, "TYPE", FALSE); g_assert (!val); diff --git a/src/settings/plugins/ifcfg-rh/utils.c b/src/settings/plugins/ifcfg-rh/utils.c index 07efa5cc53..11b8a52e8d 100644 --- a/src/settings/plugins/ifcfg-rh/utils.c +++ b/src/settings/plugins/ifcfg-rh/utils.c @@ -296,7 +296,7 @@ utils_get_extra_ifcfg (const char *parent, const char *tag, gboolean should_crea ifcfg = svCreateFile (path); if (!ifcfg) - ifcfg = svOpenFile (path); + ifcfg = svOpenFile (path, NULL); g_free (path); return ifcfg; diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c index 9c7b3037a3..1f0e6a9dda 100644 --- a/src/settings/plugins/ifcfg-rh/writer.c +++ b/src/settings/plugins/ifcfg-rh/writer.c @@ -97,6 +97,7 @@ set_secret (shvarFile *ifcfg, gboolean verbatim) { shvarFile *keyfile; + GError *error = NULL; /* Clear the secret from the ifcfg and the associated "keys" file */ svSetValue (ifcfg, key, NULL, FALSE); @@ -118,9 +119,9 @@ set_secret (shvarFile *ifcfg, if (flags == NM_SETTING_SECRET_FLAG_NONE) svSetValue (keyfile, key, value, verbatim); - if (!svWriteFile (keyfile, 0600)) { - PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: could not update key file '%s'", - keyfile->fileName); + if (!svWriteFile (keyfile, 0600, &error)) { + PLUGIN_WARN (IFCFG_PLUGIN_NAME, " warning: %s", error->message); + g_clear_error (&error); svCloseFile (keyfile); goto error; } @@ -2118,9 +2119,7 @@ write_ip4_setting (NMConnection *connection, shvarFile *ifcfg, GError **error) g_free (gw_key); g_free (metric_key); } - if (!svWriteFile (routefile, 0644)) { - g_set_error (error, IFCFG_PLUGIN_ERROR, 0, - "Could not update route file '%s'", routefile->fileName); + if (!svWriteFile (routefile, 0644, error)) { svCloseFile (routefile); goto out; } @@ -2529,7 +2528,10 @@ write_connection (NMConnection *connection, if (filename) { /* For existing connections, 'filename' should be full path to ifcfg file */ - ifcfg = svOpenFile (filename); + ifcfg = svOpenFile (filename, error); + if (!ifcfg) + return FALSE; + ifcfg_name = g_strdup (filename); } else { char *escaped; @@ -2565,12 +2567,6 @@ write_connection (NMConnection *connection, ifcfg = svCreateFile (ifcfg_name); } - if (!ifcfg) { - g_set_error (error, IFCFG_PLUGIN_ERROR, 0, - "Failed to open/create ifcfg file '%s'", ifcfg_name); - goto out; - } - type = nm_setting_connection_get_connection_type (s_con); if (!type) { g_set_error (error, IFCFG_PLUGIN_ERROR, 0, @@ -2641,11 +2637,8 @@ write_connection (NMConnection *connection, write_connection_setting (s_con, ifcfg); - if (!svWriteFile (ifcfg, 0644)) { - g_set_error (error, IFCFG_PLUGIN_ERROR, 0, - "Can't write connection '%s'", ifcfg->fileName); + if (!svWriteFile (ifcfg, 0644, error)) goto out; - } /* Only return the filename if this was a newly written ifcfg */ if (out_filename && !filename)