Some kbuild targets don't warn us about permission of the configure.
If you don't set write permission, you lose changes.
So I added warning and error.
Cc: Roman Zippel <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Arnaud Lacombe <[email protected]>
Signed-off-by: Hiromu Yakura <[email protected]>
---
scripts/kconfig/conf.c | 6 ++++++
scripts/kconfig/confdata.c | 17 +++++++++++++++++
scripts/kconfig/gconf.c | 4 ++++
scripts/kconfig/lkc.h | 1 +
scripts/kconfig/mconf.c | 4 ++++
scripts/kconfig/nconf.c | 4 ++++
scripts/kconfig/qconf.cc | 4 ++++
7 files changed, 40 insertions(+), 0 deletions(-)
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..3de1fbe 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,19 @@ 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;
+
+ name = conf_get_configname();
+ dir = dirname((char *)name);
+
+ ret = access(dir, 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
--
1.7.4.1
Hi,
On Mon, May 23, 2011 at 12:16 PM, hiromu <[email protected]> wrote:
> Some kbuild targets don't warn us about permission of the configure.
> If you don't set write permission, you lose changes.
Do you have a precise way to reproduce this, in particular which
target is involved ? I tried to `chmod 555' the kernel root directory,
re-ran `conf' (through the `defconfig' target) and `mconf' (manually
for this one, as check-lxdialog.sh fails when invoked though make).
The former failed with:
*** Error during writing of the configuration.
gmake[1]: *** [defconfig] Error 1
gmake: *** [defconfig] Error 2
so the error is also propagated through make(1) to the shell and the
latter failed with:
Error while writing of the configuration.
Your configuration changes were NOT saved.
So check _are_ being done whether or not the configuration can be
written, but there might be a corner-case not checked, in that case,
this specific path should be fixed.
Thanks,
- Arnaud
> Cc: Roman Zippel <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Arnaud Lacombe <[email protected]>
>
> Signed-off-by: Hiromu Yakura <[email protected]>
> ---
> ?scripts/kconfig/conf.c ? ? | ? ?6 ++++++
> ?scripts/kconfig/confdata.c | ? 17 +++++++++++++++++
> ?scripts/kconfig/gconf.c ? ?| ? ?4 ++++
> ?scripts/kconfig/lkc.h ? ? ?| ? ?1 +
> ?scripts/kconfig/mconf.c ? ?| ? ?4 ++++
> ?scripts/kconfig/nconf.c ? ?| ? ?4 ++++
> ?scripts/kconfig/qconf.cc ? | ? ?4 ++++
> ?7 files changed, 40 insertions(+), 0 deletions(-)
>
> 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..3de1fbe 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,19 @@ 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;
> +
> + ? ? ? name = conf_get_configname();
> + ? ? ? dir = dirname((char *)name);
> +
> + ? ? ? ret = access(dir, 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
> --
> 1.7.4.1
>
>
>
>
Hello,
thanks for replying.
On Mon, May 23, 2011 at 13:58, Arnaud Lacombe <[email protected]> wrote:
> Do you have a precise way to reproduce this, in particular which
> target is involved ? I tried to `chmod 555' the kernel root directory,
> re-ran `conf' (through the `defconfig' target) and `mconf' (manually
> for this one, as check-lxdialog.sh fails when invoked though make).
> The former failed with:
>
> *** Error during writing of the configuration.
>
> gmake[1]: *** [defconfig] Error 1
> gmake: *** [defconfig] Error 2
In my environment, this patch is working properly.
The output is as follows:
hiromu@hiromu-MacBook:/usr/src/linux-2.6$ ls -ld .
dr-xr-sr-x 25 hiromu hiromu 4096 May 24 21:41 .
hiromu@hiromu-MacBook:/usr/src/linux-2.6$ make defconfig
*** Default configuration is based on 'x86_64_defconfig'
*** Permission denied to write the configuration.
make[1]: *** [defconfig] Error 1
make: *** [defconfig] Error 2
hiromu@hiromu-MacBook:/usr/src/linux-2.6$
> So check _are_ being done whether or not the configuration can be
> written, but there might be a corner-case not checked, in that case,
> this specific path should be fixed.
I think it is OK if you have the write permission to the directory
which be saved the configuration.
> > Cc: Roman Zippel <[email protected]>
> > Cc: Michal Marek <[email protected]>
> > Cc: Arnaud Lacombe <[email protected]>
> >
> > Signed-off-by: Hiromu Yakura <[email protected]>
> > ---
> > scripts/kconfig/conf.c | 6 ++++++
> > scripts/kconfig/confdata.c | 17 +++++++++++++++++
> > scripts/kconfig/gconf.c | 4 ++++
> > scripts/kconfig/lkc.h | 1 +
> > scripts/kconfig/mconf.c | 4 ++++
> > scripts/kconfig/nconf.c | 4 ++++
> > scripts/kconfig/qconf.cc | 4 ++++
> > 7 files changed, 40 insertions(+), 0 deletions(-)
> >
> > 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..3de1fbe 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,19 @@ 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;
> > +
> > + name = conf_get_configname();
> > + dir = dirname((char *)name);
> > +
> > + ret = access(dir, 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
> > --
> > 1.7.4.1
> >
> >
> >
> >
On 24.5.2011 15:06, Hiromu Yakura wrote:
> Hello,
> thanks for replying.
>
> On Mon, May 23, 2011 at 13:58, Arnaud Lacombe<[email protected]> wrote:
>> Do you have a precise way to reproduce this, in particular which
>> target is involved ? I tried to `chmod 555' the kernel root directory,
>> re-ran `conf' (through the `defconfig' target) and `mconf' (manually
>> for this one, as check-lxdialog.sh fails when invoked though make).
>> The former failed with:
>>
>> *** Error during writing of the configuration.
>>
>> gmake[1]: *** [defconfig] Error 1
>> gmake: *** [defconfig] Error 2
> In my environment, this patch is working properly.
> The output is as follows:
>
> hiromu@hiromu-MacBook:/usr/src/linux-2.6$ ls -ld .
> dr-xr-sr-x 25 hiromu hiromu 4096 May 24 21:41 .
> hiromu@hiromu-MacBook:/usr/src/linux-2.6$ make defconfig
> *** Default configuration is based on 'x86_64_defconfig'
> *** Permission denied to write the configuration.
Arnaud's point is that your patch should not be necessary at all,
because kconfig already checks the return value of the fopen() call in
conf_write() and prints the above message if it fails. So do you have a
testcase where make <...>config without your patch returns without
error, but the configuration is not written?
Michal
Hello,
On Fri, May 24, 2011 at 22:23, Michal Marek <[email protected]> wrote:
> Arnaud's point is that your patch should not be necessary at all,
> because kconfig already checks the return value of the fopen() call in
> conf_write() and prints the above message if it fails. So do you have a
> testcase where make <...>config without your patch returns without
> error, but the configuration is not written?
Sorry for misunderstanding.
Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
raise error.
And only xconfig and gconfig don't write the configuration without
error.
However, make *config using 'conf' hasn't function of 'Save as'.
In other words, if you don't set the write permission to the directory,
you would lose changes of the configuration.
I think that it is necessary to check permission at all so that kconfig
warn of the possibility of losing changes.
Thanks
On 24.5.2011 16:26, Hiromu Yakura wrote:
> Hello,
>
> On Fri, May 24, 2011 at 22:23, Michal Marek<[email protected]> wrote:
>> Arnaud's point is that your patch should not be necessary at all,
>> because kconfig already checks the return value of the fopen() call in
>> conf_write() and prints the above message if it fails. So do you have a
>> testcase where make<...>config without your patch returns without
>> error, but the configuration is not written?
> Sorry for misunderstanding.
> Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
> raise error.
> And only xconfig and gconfig don't write the configuration without
> error.
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. 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.
Michal
Hi,
On Tue, May 24, 2011 at 11:01 AM, Michal Marek <[email protected]> wrote:
> On 24.5.2011 16:26, Hiromu Yakura wrote:
>>
>> Hello,
>>
>> On Fri, May 24, 2011 at 22:23, Michal Marek<[email protected]> ?wrote:
>>>
>>> Arnaud's point is that your patch should not be necessary at all,
>>> because kconfig already checks the return value of the fopen() call in
>>> conf_write() and prints the above message if it fails. So do you have a
>>> testcase where make<...>config without your patch returns without
>>> error, but the configuration is not written?
>>
>> Sorry for misunderstanding.
>> Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
>> raise error.
>> And only xconfig and gconfig don't write the configuration without
>> error.
>
> 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.
- Arnaud
> >>>
> >>> Arnaud's point is that your patch should not be necessary at all,
> >>> because kconfig already checks the return value of the fopen() call in
> >>> conf_write() and prints the above message if it fails. So do you have a
> >>> testcase where make<...>config without your patch returns without
> >>> error, but the configuration is not written?
> >>
> >> Sorry for misunderstanding.
> >> Indeed, make *config which use 'conf' (e.g. oldconfig, defconfig...)
> >> raise error.
> >> And only xconfig and gconfig don't write the configuration without
> >> error.
> >
> > 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.
The only thing that a write-access check up-front could be useful for would be
to launch a front-end in ReadOnly mode.
And this is not supported by any of them today IIRC.
Sam
On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <[email protected]>
wrote:
> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <[email protected]> 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 <[email protected]>
---
scripts/kconfig/conf.c | 6 ++++++
scripts/kconfig/confdata.c | 24 ++++++++++++++++++++++++
scripts/kconfig/gconf.c | 4 ++++
scripts/kconfig/lkc.h | 1 +
scripts/kconfig/mconf.c | 4 ++++
scripts/kconfig/nconf.c | 4 ++++
scripts/kconfig/qconf.cc | 4 ++++
7 files changed, 47 insertions(+), 0 deletions(-)
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
--
1.7.4.1
Hi,
On Tue, May 24, 2011 at 1:38 PM, Hiromu Yakura <[email protected]> wrote:
> On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <[email protected]>
> wrote:
>> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <[email protected]> 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 <[email protected]>
>
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
On Tue, May 24, 2011, at 2:59, Arnaud Lacombe <[email protected]>
wrote:
> Hi,
>
> On Tue, May 24, 2011 at 1:38 PM, Hiromu Yakura <[email protected]> wrote:
> > On Tue, May 24, 2011, at 0:50, Arnaud Lacombe <[email protected]>
> > wrote:
> >> On Tue, May 24, 2011 at 11:01 AM, Michal Marek <[email protected]> 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 <[email protected]>
> >
> 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.
Hi,
On Tue, May 24, 2011 at 4:46 PM, Hiromu Yakura <[email protected]> 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...