| Message ID | 1426243805-8317-2-git-send-email-angelo.compagnucci@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Buildroot developers, Il 13/mar/2015 11:50, "Angelo Compagnucci" <angelo.compagnucci@gmail.com> ha scritto: > > This patch adds the possibilty to change owner/permission > of a folder recursively. > > Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> > --- Anybody willing to review this patch? I think it's very important to have the option to set permission on folders recursively! I think this patch is really simple and could be reviewed/accepted with a minimal effort. > package/makedevs/makedevs.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c > index ab90b93..e575427 100644 > --- a/package/makedevs/makedevs.c > +++ b/package/makedevs/makedevs.c > @@ -34,6 +34,8 @@ > #ifndef __APPLE__ > #include <sys/sysmacros.h> /* major() and minor() */ > #endif > +#include <ftw.h> > + > > const char *bb_applet_name; > > @@ -332,6 +334,7 @@ void bb_show_usage(void) > fprintf(stderr, "Where name is the file name, type can be one of:\n"); > fprintf(stderr, " f A regular file\n"); > fprintf(stderr, " d Directory\n"); > + fprintf(stderr, " r Directory recursively\n"); > fprintf(stderr, " c Character special device file\n"); > fprintf(stderr, " b Block special device file\n"); > fprintf(stderr, " p Fifo (named pipe)\n"); > @@ -364,6 +367,20 @@ void bb_show_usage(void) > exit(1); > } > > +bb_recursive(const char *full_name, uid_t uid, gid_t gid, unsigned int mode){ > + > + int chmod_chown(const char *fpath, const struct stat *sb, > + int tflag, struct FTW *ftwbuf) { > + if (chown(fpath, uid, gid) == -1) return -1; > + if ((mode != -1)) { > + if (chmod(fpath, mode) < 0) return -1; > + } > + return 0; > + } > + > + return nftw(full_name, chmod_chown, 20, FTW_MOUNT | FTW_PHYS); > +} > + > int main(int argc, char **argv) > { > int opt; > @@ -474,6 +491,12 @@ int main(int argc, char **argv) > ret = EXIT_FAILURE; > goto loop; > } > + } else if (type == 'r') { > + if (bb_recursive(full_name, uid, gid, mode) < 0) { > + bb_perror_msg("line %d: recursive failed for %s", linenum, full_name); > + ret = EXIT_FAILURE; > + goto loop; > + } > } else > { > dev_t rdev; > -- > 1.9.1 >
>>>>> "Angelo" == Angelo Compagnucci <angelo.compagnucci@gmail.com> writes: > Dear Buildroot developers, > Il 13/mar/2015 11:50, "Angelo Compagnucci" <angelo.compagnucci@gmail.com> > ha scritto: >> >> This patch adds the possibilty to change owner/permission >> of a folder recursively. >> >> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> >> --- > Anybody willing to review this patch? I think it's very important to have > the option to set permission on folders recursively! > I think this patch is really simple and could be reviewed/accepted with a > minimal effort. Sorry, I've been away quite a bit lately, I'll try to find time to review later today.
Angelo, All, On 2015-03-13 11:50 +0100, Angelo Compagnucci spake thusly: > This patch adds the possibilty to change owner/permission > of a folder recursively. > > Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> [I really only did a review and no actual tests] As a side comment: maybe that would be interesting to submit a similar patch for busybox (where makedevs originated); note however that busybox has changed quite a lot since we forked makedevs... Regards, Yann E. MORIN. > --- > package/makedevs/makedevs.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c > index ab90b93..e575427 100644 > --- a/package/makedevs/makedevs.c > +++ b/package/makedevs/makedevs.c > @@ -34,6 +34,8 @@ > #ifndef __APPLE__ > #include <sys/sysmacros.h> /* major() and minor() */ > #endif > +#include <ftw.h> > + > > const char *bb_applet_name; > > @@ -332,6 +334,7 @@ void bb_show_usage(void) > fprintf(stderr, "Where name is the file name, type can be one of:\n"); > fprintf(stderr, " f A regular file\n"); > fprintf(stderr, " d Directory\n"); > + fprintf(stderr, " r Directory recursively\n"); > fprintf(stderr, " c Character special device file\n"); > fprintf(stderr, " b Block special device file\n"); > fprintf(stderr, " p Fifo (named pipe)\n"); > @@ -364,6 +367,20 @@ void bb_show_usage(void) > exit(1); > } > > +bb_recursive(const char *full_name, uid_t uid, gid_t gid, unsigned int mode){ > + > + int chmod_chown(const char *fpath, const struct stat *sb, > + int tflag, struct FTW *ftwbuf) { > + if (chown(fpath, uid, gid) == -1) return -1; > + if ((mode != -1)) { > + if (chmod(fpath, mode) < 0) return -1; > + } > + return 0; > + } > + > + return nftw(full_name, chmod_chown, 20, FTW_MOUNT | FTW_PHYS); > +} > + > int main(int argc, char **argv) > { > int opt; > @@ -474,6 +491,12 @@ int main(int argc, char **argv) > ret = EXIT_FAILURE; > goto loop; > } > + } else if (type == 'r') { > + if (bb_recursive(full_name, uid, gid, mode) < 0) { > + bb_perror_msg("line %d: recursive failed for %s", linenum, full_name); > + ret = EXIT_FAILURE; > + goto loop; > + } > } else > { > dev_t rdev; > -- > 1.9.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Dear Yann E. MORIN, 2015-04-09 23:23 GMT+02:00 Yann E. MORIN <yann.morin.1998@free.fr>: > Angelo, All, > > On 2015-03-13 11:50 +0100, Angelo Compagnucci spake thusly: >> This patch adds the possibilty to change owner/permission >> of a folder recursively. >> >> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> > > Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > [I really only did a review and no actual tests] > > As a side comment: maybe that would be interesting to submit a similar > patch for busybox (where makedevs originated); note however that busybox > has changed quite a lot since we forked makedevs... I've not submitted upstream cause the code for makedevs is in buildroot source tree ... If you think it's important, I will submit not later the patch will accepted. Thank you! > > Regards, > Yann E. MORIN. > >> --- >> package/makedevs/makedevs.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c >> index ab90b93..e575427 100644 >> --- a/package/makedevs/makedevs.c >> +++ b/package/makedevs/makedevs.c >> @@ -34,6 +34,8 @@ >> #ifndef __APPLE__ >> #include <sys/sysmacros.h> /* major() and minor() */ >> #endif >> +#include <ftw.h> >> + >> >> const char *bb_applet_name; >> >> @@ -332,6 +334,7 @@ void bb_show_usage(void) >> fprintf(stderr, "Where name is the file name, type can be one of:\n"); >> fprintf(stderr, " f A regular file\n"); >> fprintf(stderr, " d Directory\n"); >> + fprintf(stderr, " r Directory recursively\n"); >> fprintf(stderr, " c Character special device file\n"); >> fprintf(stderr, " b Block special device file\n"); >> fprintf(stderr, " p Fifo (named pipe)\n"); >> @@ -364,6 +367,20 @@ void bb_show_usage(void) >> exit(1); >> } >> >> +bb_recursive(const char *full_name, uid_t uid, gid_t gid, unsigned int mode){ >> + >> + int chmod_chown(const char *fpath, const struct stat *sb, >> + int tflag, struct FTW *ftwbuf) { >> + if (chown(fpath, uid, gid) == -1) return -1; >> + if ((mode != -1)) { >> + if (chmod(fpath, mode) < 0) return -1; >> + } >> + return 0; >> + } >> + >> + return nftw(full_name, chmod_chown, 20, FTW_MOUNT | FTW_PHYS); >> +} >> + >> int main(int argc, char **argv) >> { >> int opt; >> @@ -474,6 +491,12 @@ int main(int argc, char **argv) >> ret = EXIT_FAILURE; >> goto loop; >> } >> + } else if (type == 'r') { >> + if (bb_recursive(full_name, uid, gid, mode) < 0) { >> + bb_perror_msg("line %d: recursive failed for %s", linenum, full_name); >> + ret = EXIT_FAILURE; >> + goto loop; >> + } >> } else >> { >> dev_t rdev; >> -- >> 1.9.1 >> >> _______________________________________________ >> buildroot mailing list >> buildroot@busybox.net >> http://lists.busybox.net/mailman/listinfo/buildroot > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------'
Angelo, All, On 2015-04-10 07:39 +0200, Angelo Compagnucci spake thusly: > 2015-04-09 23:23 GMT+02:00 Yann E. MORIN <yann.morin.1998@free.fr>: > > On 2015-03-13 11:50 +0100, Angelo Compagnucci spake thusly: > >> This patch adds the possibilty to change owner/permission > >> of a folder recursively. > >> > >> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> > > > > Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > > > [I really only did a review and no actual tests] > > > > As a side comment: maybe that would be interesting to submit a similar > > patch for busybox (where makedevs originated); note however that busybox > > has changed quite a lot since we forked makedevs... > > I've not submitted upstream cause the code for makedevs is in > buildroot source tree ... > If you think it's important, I will submit not later the patch will accepted. Well, I won't say it is "important". It all really depends from the perspective. For one, I believe this is an interesting feature, from which busybox could well benefit. So it makes sense to submit it to busybox. Of course, this is not really correlated to our own makedevs, except by ancestry. So, if you have spare CPU cycles, please go ahead. If not, then no problem. That was just a suggestion! ;-) Thanks! Regards, Yann E. MORIN.
>>>>> "Angelo" == Angelo Compagnucci <angelo.compagnucci@gmail.com> writes: > This patch adds the possibilty to change owner/permission > of a folder recursively. Thanks, a few comments: > +bb_recursive(const char *full_name, uid_t uid, gid_t gid, unsigned int mode){ > + > + int chmod_chown(const char *fpath, const struct stat *sb, > + int tflag, struct FTW *ftwbuf) { > + if (chown(fpath, uid, gid) == -1) return -1; > + if ((mode != -1)) { > + if (chmod(fpath, mode) < 0) return -1; > + } > + return 0; > + } I know you are doing this because nftw doesn't allow any extra arguments to be passed to the function, but using GCC specific nested functions isn't really nice. Does E.G. clang support these? Alternatively we could port recursive_action() from busybox. It would also be good to stick some bb_error_msg in there so people can get a hint about what fails.
Dear Peter, 2015-04-10 23:41 GMT+02:00 Peter Korsgaard <peter@korsgaard.com>: >>>>>> "Angelo" == Angelo Compagnucci <angelo.compagnucci@gmail.com> writes: > > > This patch adds the possibilty to change owner/permission > > of a folder recursively. > > Thanks, a few comments: > > > +bb_recursive(const char *full_name, uid_t uid, gid_t gid, unsigned int mode){ > > + > > + int chmod_chown(const char *fpath, const struct stat *sb, > > + int tflag, struct FTW *ftwbuf) { > > + if (chown(fpath, uid, gid) == -1) return -1; > > + if ((mode != -1)) { > > + if (chmod(fpath, mode) < 0) return -1; > > + } > > + return 0; > > + } > > I know you are doing this because nftw doesn't allow any extra arguments > to be passed to the function, but using GCC specific nested functions > isn't really nice. > Does E.G. clang support these? Doh! I thought nested function could be a more widespread feature! Of course, you are right, clang doesn't support nested functions. The naive solution could be to add a global object, could be acceptable? There are only three variables and could be nested inside a nice structure. > Alternatively we could port > recursive_action() from busybox. / * Unfortunately, while nftw(3) could replace this and reduce * code size a bit, nftw() wasn't supported before GNU libc 2.1, * and so isn't sufficiently portable to take over since glibc2.1 * is so stinking huge. */ The only reason why they stick with an hand made recursive function instead of nftw is to support older glibc 2.1 (!) that doesn't have that function. I think it's not our problem and backporting that old code is not a good idea! What do you think? > It would also be good to stick some bb_error_msg in there so people can > get a hint about what fails. Ok! > > -- > Bye, Peter Korsgaard
>>>>> "Angelo" == Angelo Compagnucci <angelo.compagnucci@gmail.com> writes: hi, >> I know you are doing this because nftw doesn't allow any extra arguments >> to be passed to the function, but using GCC specific nested functions >> isn't really nice. >> Does E.G. clang support these? > Doh! I thought nested function could be a more widespread feature! Of > course, you are right, clang doesn't support nested functions. > The naive solution could be to add a global object, could be acceptable? > There are only three variables and could be nested inside a nice structure. Yes, or simply 3 globals (recursive_{uid,gid,mode}) to keep it simple. >> Alternatively we could port >> recursive_action() from busybox. > / * Unfortunately, while nftw(3) could replace this and reduce > * code size a bit, nftw() wasn't supported before GNU libc 2.1, > * and so isn't sufficiently portable to take over since glibc2.1 > * is so stinking huge. > */ > The only reason why they stick with an hand made recursive function > instead of nftw is to support older glibc 2.1 (!) that doesn't have > that function. > I think it's not our problem and backporting that old code is not a good idea! Yes, that AND the fact that recursive_action takes a userData structure that gets forwarded to the callbacks. But yeah, just using nftw with 3 globals is simpler.
diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c index ab90b93..e575427 100644 --- a/package/makedevs/makedevs.c +++ b/package/makedevs/makedevs.c @@ -34,6 +34,8 @@ #ifndef __APPLE__ #include <sys/sysmacros.h> /* major() and minor() */ #endif +#include <ftw.h> + const char *bb_applet_name; @@ -332,6 +334,7 @@ void bb_show_usage(void) fprintf(stderr, "Where name is the file name, type can be one of:\n"); fprintf(stderr, " f A regular file\n"); fprintf(stderr, " d Directory\n"); + fprintf(stderr, " r Directory recursively\n"); fprintf(stderr, " c Character special device file\n"); fprintf(stderr, " b Block special device file\n"); fprintf(stderr, " p Fifo (named pipe)\n"); @@ -364,6 +367,20 @@ void bb_show_usage(void) exit(1); } +bb_recursive(const char *full_name, uid_t uid, gid_t gid, unsigned int mode){ + + int chmod_chown(const char *fpath, const struct stat *sb, + int tflag, struct FTW *ftwbuf) { + if (chown(fpath, uid, gid) == -1) return -1; + if ((mode != -1)) { + if (chmod(fpath, mode) < 0) return -1; + } + return 0; + } + + return nftw(full_name, chmod_chown, 20, FTW_MOUNT | FTW_PHYS); +} + int main(int argc, char **argv) { int opt; @@ -474,6 +491,12 @@ int main(int argc, char **argv) ret = EXIT_FAILURE; goto loop; } + } else if (type == 'r') { + if (bb_recursive(full_name, uid, gid, mode) < 0) { + bb_perror_msg("line %d: recursive failed for %s", linenum, full_name); + ret = EXIT_FAILURE; + goto loop; + } } else { dev_t rdev;
This patch adds the possibilty to change owner/permission of a folder recursively. Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> --- package/makedevs/makedevs.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)