| Message ID | 1306258716.7501.35.camel@hiromu-MacBook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, May 24, 2011 at 1:38 PM, Hiromu Yakura <hiromu1996@gmail.com> wrote: > On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <lacombar@gmail.com> > wrote: >> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <mmarek@suse.cz> wrote: >> > I see, qconf lacks a check for the return value of conf_write() in >> > ConfigMainWindow::closeEvent(), gconf does check the return value, but only >> > displays it in the bottom box of the main window instead of a message box. >> > Neither of them return failure in the error case. These bugs should be >> > indeed fixed. >> > >> agree. >> >> > But I don't like the directory permission check, it only >> > handles one case, but does not handle permission on the .config file itself >> > with KCONFIG_OVERWRITECONFIG=1, ENOSPC and so on. >> > >> seconded. > I'm sorry for forgetting to handle a case which was set KCONFIG_OVERWRITECONFIG. > So I rewrote the patch and attach it. > > Thanks for your advice. > > Signed-off-by: Hiromu Yakura <hiromu1996@gmail.com> > Let me re-state: your patch does not handle all the case where conf_write() may fails, and I do not think we want to preemptively check for all errors open(2) may return. - Arnaud -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 24, 2011, at 2:59, Arnaud Lacombe <lacombar@gmail.com> wrote: > Hi, > > On Tue, May 24, 2011 at 1:38 PM, Hiromu Yakura <hiromu1996@gmail.com> wrote: > > On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <lacombar@gmail.com> > > wrote: > >> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <mmarek@suse.cz> wrote: > >> > I see, qconf lacks a check for the return value of conf_write() in > >> > ConfigMainWindow::closeEvent(), gconf does check the return value, but only > >> > displays it in the bottom box of the main window instead of a message box. > >> > Neither of them return failure in the error case. These bugs should be > >> > indeed fixed. > >> > > >> agree. > >> > >> > But I don't like the directory permission check, it only > >> > handles one case, but does not handle permission on the .config file itself > >> > with KCONFIG_OVERWRITECONFIG=1, ENOSPC and so on. > >> > > >> seconded. > > I'm sorry for forgetting to handle a case which was set KCONFIG_OVERWRITECONFIG. > > So I rewrote the patch and attach it. > > > > Thanks for your advice. > > > > Signed-off-by: Hiromu Yakura <hiromu1996@gmail.com> > > > Let me re-state: your patch does not handle all the case where > conf_write() may fails, and I do not think we want to preemptively > check for all errors open(2) may return. conf_write() is called after the configure changed. So I don't think we should handle the failed case of conf_write() because the purpose of this patch is not to losing changes. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, May 24, 2011 at 4:46 PM, Hiromu Yakura <hiromu1996@gmail.com> wrote: > [...] > conf_write() is called after the configure changed. > So I don't think we should handle the failed case of conf_write() > because the purpose of this patch is not to losing changes. > Then your patch is incomplete. only open(2) and write(2) can fail in more than 30 different ways, you check 1. - Arnaud ps: I am not even addressing the race-condition introduced by your patch... -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c index 006ad81..d93e351 100644 --- a/scripts/kconfig/conf.c +++ b/scripts/kconfig/conf.c @@ -466,6 +466,12 @@ int main(int ac, char **av) bindtextdomain(PACKAGE, LOCALEDIR); textdomain(PACKAGE); + if (conf_check_permission()) { + fprintf(stderr, + "*** Permission denied to write the configuration.\n\n"); + exit(1); + } + while ((opt = getopt_long(ac, av, "", long_opts, NULL)) != -1) { input_mode = (enum input_mode)opt; switch (opt) { diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 61c35bf..2070ac0 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -7,6 +7,7 @@ #include <ctype.h> #include <errno.h> #include <fcntl.h> +#include <libgen.h> #include <stdio.h> #include <stdlib.h> #include <string.h> @@ -1051,3 +1052,26 @@ void conf_set_all_new_symbols(enum conf_def_mode mode) set_all_choice_values(csym); } } + +int conf_check_permission(void) +{ + int ret, retval = 0; + const char *name; + char *dir, *env; + + name = conf_get_configname(); + + env = getenv("KCONFIG_OVERWRITECONFIG"); + if (!env || !*env) { + dir = dirname((char *)name); + ret = access(dir, W_OK); + if (ret < 0) + retval = -errno; + } else { + ret = access(name, W_OK); + if (ret < 0) + retval = -errno; + } + + return retval; +} diff --git a/scripts/kconfig/gconf.c b/scripts/kconfig/gconf.c index 4558961..3567a23 100644 --- a/scripts/kconfig/gconf.c +++ b/scripts/kconfig/gconf.c @@ -1510,6 +1510,10 @@ int main(int ac, char *av[]) bind_textdomain_codeset(PACKAGE, "UTF-8"); textdomain(PACKAGE); + if (conf_check_permission()) + fprintf(stderr, + "Warning: Permission denied to write the configuration.\n"); + /* GTK stuffs */ gtk_set_locale(); gtk_init(&ac, &av); diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h index febf0c9..4d20841 100644 --- a/scripts/kconfig/lkc.h +++ b/scripts/kconfig/lkc.h @@ -91,6 +91,7 @@ char *conf_get_default_confname(void); void sym_set_change_count(int count); void sym_add_change_count(int count); void conf_set_all_new_symbols(enum conf_def_mode mode); +int conf_check_permission(void); /* confdata.c and expr.c */ static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out) diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c index d433c7a..c820e05 100644 --- a/scripts/kconfig/mconf.c +++ b/scripts/kconfig/mconf.c @@ -803,6 +803,10 @@ int main(int ac, char **av) bindtextdomain(PACKAGE, LOCALEDIR); textdomain(PACKAGE); + if (conf_check_permission()) + fprintf(stderr, + "Warning: Permission denied to write the configuration.\n"); + conf_parse(av[1]); conf_read(NULL); diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c index db56377..1cea031 100644 --- a/scripts/kconfig/nconf.c +++ b/scripts/kconfig/nconf.c @@ -1491,6 +1491,10 @@ int main(int ac, char **av) bindtextdomain(PACKAGE, LOCALEDIR); textdomain(PACKAGE); + if (conf_check_permission()) + fprintf(stderr, + "Warning: Permission denied to write the configuration.\n"); + conf_parse(av[1]); conf_read(NULL); diff --git a/scripts/kconfig/qconf.cc b/scripts/kconfig/qconf.cc index 06dd2e3..7dca7ac 100644 --- a/scripts/kconfig/qconf.cc +++ b/scripts/kconfig/qconf.cc @@ -1746,6 +1746,10 @@ int main(int ac, char** av) bindtextdomain(PACKAGE, LOCALEDIR); textdomain(PACKAGE); + if (conf_check_permission()) + fprintf(stderr, + "Warning: Permission denied to write the configuration.\n"); + #ifndef LKC_DIRECT_LINK kconfig_load(); #endif