Hi!

Thanks for the comments. Please see my responses below.

On Wed, 2015-03-04 at 16:17 +0100, Bartosz Gołaszewski wrote:
> 2015-03-04 15:11 GMT+01:00 Ari Sundholm <[email protected]>:
> > From: Ari Sundholm <[email protected]>
> >
> > bloat-o-meter:
> >
> > function                                             old     new   delta
> > .rodata                                           154411  154491     +80
> > ------------------------------------------------------------------------------
> > (add/remove: 0/0 grow/shrink: 1/0 up/down: 80/0)               Total: 80 
> > bytes
> >
> > v2: Make dd and truncate share a common suffix struct.
> >
> > Signed-off-by: Ari Sundholm <[email protected]>
> > ---
> >  coreutils/dd.c       | 31 +++++-----------------
> >  coreutils/truncate.c | 73 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/libbb.h      |  1 +
> >  libbb/xatonum.c      | 19 ++++++++++++++
> >  4 files changed, 99 insertions(+), 25 deletions(-)
> >  create mode 100644 coreutils/truncate.c
> >
> > diff --git a/coreutils/dd.c b/coreutils/dd.c
> > index 3024970..53a843c 100644
> > --- a/coreutils/dd.c
> > +++ b/coreutils/dd.c
> > @@ -99,25 +99,6 @@ enum {
> >         ofd = STDOUT_FILENO,
> >  };
> >
> > -static const struct suffix_mult dd_suffixes[] = {
> > -       { "c", 1 },
> > -       { "w", 2 },
> > -       { "b", 512 },
> > -       { "kB", 1000 },
> > -       { "kD", 1000 },
> > -       { "k", 1024 },
> > -       { "K", 1024 },  /* compat with coreutils dd (it also accepts KB and 
> > KD, TODO?) */
> > -       { "MB", 1000000 },
> > -       { "MD", 1000000 },
> > -       { "M", 1024*1024 },
> > -       { "GB", 1000000000 },
> > -       { "GD", 1000000000 },
> > -       { "G", 1024*1024*1024 },
> > -       /* "D" suffix for decimal is not in coreutils manpage, looks like 
> > it's deprecated */
> > -       /* coreutils also understands TPEZY suffixes for tera- and so on, 
> > with B suffix for decimal */
> > -       { "", 0 }
> > -};
> > -
> >  struct globals {
> >         off_t out_full, out_part, in_full, in_part;
> >  #if ENABLE_FEATURE_DD_THIRD_STATUS_LINE
> > @@ -326,11 +307,11 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
> >  #if ENABLE_FEATURE_DD_IBS_OBS
> >                 if (what == OP_ibs) {
> >                         /* Must fit into positive ssize_t */
> > -                       ibs = xatoul_range_sfx(val, 1, ((size_t)-1L)/2, 
> > dd_suffixes);
> > +                       ibs = xatoul_range_sfx(val, 1, ((size_t)-1L)/2, 
> > cwbkMG_suffixes);
> >                         /*continue;*/
> >                 }
> >                 if (what == OP_obs) {
> > -                       obs = xatoul_range_sfx(val, 1, ((size_t)-1L)/2, 
> > dd_suffixes);
> > +                       obs = xatoul_range_sfx(val, 1, ((size_t)-1L)/2, 
> > cwbkMG_suffixes);
> >                         /*continue;*/
> >                 }
> >                 if (what == OP_conv) {
> > @@ -356,22 +337,22 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
> >                 }
> >  #endif
> >                 if (what == OP_bs) {
> > -                       ibs = xatoul_range_sfx(val, 1, ((size_t)-1L)/2, 
> > dd_suffixes);
> > +                       ibs = xatoul_range_sfx(val, 1, ((size_t)-1L)/2, 
> > cwbkMG_suffixes);
> >                         obs = ibs;
> >                         /*continue;*/
> >                 }
> >                 /* These can be large: */
> >                 if (what == OP_count) {
> >                         G.flags |= FLAG_COUNT;
> > -                       count = XATOU_SFX(val, dd_suffixes);
> > +                       count = XATOU_SFX(val, cwbkMG_suffixes);
> >                         /*continue;*/
> >                 }
> >                 if (what == OP_seek) {
> > -                       seek = XATOU_SFX(val, dd_suffixes);
> > +                       seek = XATOU_SFX(val, cwbkMG_suffixes);
> >                         /*continue;*/
> >                 }
> >                 if (what == OP_skip) {
> > -                       skip = XATOU_SFX(val, dd_suffixes);
> > +                       skip = XATOU_SFX(val, cwbkMG_suffixes);
> >                         /*continue;*/
> >                 }
> >                 if (what == OP_if) {
> > diff --git a/coreutils/truncate.c b/coreutils/truncate.c
> > new file mode 100644
> > index 0000000..36b2f2c
> > --- /dev/null
> > +++ b/coreutils/truncate.c
> > @@ -0,0 +1,73 @@
> > +/*
> > + * Mini truncate implementation for busybox
> > + *
> > + * Copyright (C) 2015 by Ari Sundholm <[email protected]>
> > + *
> > + * Licensed under GPLv2 or later, see file LICENSE in this source tree.
> > + */
> > +
> > +//config:config TRUNCATE
> > +//config:      bool "truncate"
> > +//config:      default y
> > +//config:      help
> > +//config:        truncate truncates files to a given size. If a file does
> > +//config:        not exist, it is created unless told otherwise.
> > +
> > +//kbuild:lib-$(CONFIG_TRUNCATE) += truncate.o
> > +//applet:IF_TRUNCATE(APPLET(truncate, BB_DIR_USR_BIN, BB_SUID_DROP))
> 
> This looks like a good candidate for a NOFORK applet.
> 

Indeed. Will be done in v3.

> > +
> > +//usage:#define truncate_trivial_usage
> > +//usage:       "[-c] -s SIZE FILE..."
> > +//usage:#define truncate_full_usage "\n\n"
> > +//usage:       "Truncate FILEs to the given size.\n"
> > +//usage:       "\n     -c      Do not create any files."
> > +//usage:       "\n     -s SIZE Truncate to SIZE."
> > +//usage:
> > +//usage:#define truncate_example_usage
> > +//usage:       "$ truncate -s 1G foo"
> > +
> > +#include "libbb.h"
> > +#if ENABLE_LFS
> > +# define XATOU_SFX xatoull_sfx
> > +#else
> > +# define XATOU_SFX xatoul_sfx
> > +#endif
> > +
> > +int truncate_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
> > +int truncate_main(int argc UNUSED_PARAM, char **argv) {
> > +       unsigned opts;
> > +       int flags = O_CREAT|O_RDWR;
> > +       int fd;
> > +       char *size_str;
> > +       off_t size;
> > +
> > +       enum {
> > +               OPT_NOCREATE  = (1 << 0),
> > +               OPT_SIZE = (1 << 1),
> > +       };
> > +
> > +       opt_complementary = "s";
> > +       opts = getopt32(argv, "cs:", &size_str);
> > +
> > +       argv += optind;
> > +       if (!*argv) {
> > +               bb_error_msg("truncate: no files specified!\n");
> > +               return 1;
> 
> bb_perror_msg_and_die()?
> 

Would we not get an unrelated error string appended?

By changing the above to bb_perror_msg_and_die("no files specified"):

$ ./busybox truncate -c -s 1M
truncate: no files specified: No such file or directory

... which is a bit weird.

Maybe bb_error_msg() followed by xfunc_die()?

> > +       }
> > +
> > +       if (opts & OPT_NOCREATE)
> > +               flags = O_RDWR;
> > +
> > +       size = XATOU_SFX(size_str, cwbkMG_suffixes);
> > +
> > +       while (*argv) {
> > +               fd = xopen(*argv, flags);
> > +               if (ftruncate(fd, size) == -1)
> > +                       bb_perror_msg_and_die("ftruncate failed");
> > +               xclose(fd);
> > +               ++argv;
> > +       }
> > +
> > +       return 0;
> > +}
> > diff --git a/include/libbb.h b/include/libbb.h
> > index be792d6..e4fcf7c 100644
> > --- a/include/libbb.h
> > +++ b/include/libbb.h
> > @@ -861,6 +861,7 @@ struct suffix_mult {
> >  };
> >  extern const struct suffix_mult bkm_suffixes[];
> >  #define km_suffixes (bkm_suffixes + 1)
> > +extern const struct suffix_mult cwbkMG_suffixes[];
> >
> >  #include "xatonum.h"
> >  /* Specialized: */
> > diff --git a/libbb/xatonum.c b/libbb/xatonum.c
> > index 6f4e023..19b54fb 100644
> > --- a/libbb/xatonum.c
> > +++ b/libbb/xatonum.c
> > @@ -75,3 +75,22 @@ const struct suffix_mult bkm_suffixes[] = {
> >         { "m", 1024*1024 },
> >         { "", 0 }
> >  };
> > +
> > +const struct suffix_mult cwbkMG_suffixes[] = {
> > +       { "c", 1 },
> > +       { "w", 2 },
> > +       { "b", 512 },
> > +       { "kB", 1000 },
> > +       { "kD", 1000 },
> > +       { "k", 1024 },
> > +       { "K", 1024 },  /* compat with coreutils dd (it also accepts KB and 
> > KD, TODO?) */
> > +       { "MB", 1000000 },
> > +       { "MD", 1000000 },
> > +       { "M", 1024*1024 },
> > +       { "GB", 1000000000 },
> > +       { "GD", 1000000000 },
> > +       { "G", 1024*1024*1024 },
> > +       /* "D" suffix for decimal is not in coreutils manpage, looks like 
> > it's deprecated */
> > +       /* coreutils also understands TPEZY suffixes for tera- and so on, 
> > with B suffix for decimal */
> > +       { "", 0 }
> > +};
> > --
> 
> Maybe replace bkm_suffixes by cwbkMG_suffixes globally?
> 

We may need to be careful. libbb.h contains the following:

#define km_suffixes (bkm_suffixes + 1)

svlogd uses km_suffixes and a few applets use bkm_suffixes. These may be
intentionally using a more restricted set of suffixes. I am not quite
sure what to do here.

Maybe Denys could give his opinion on this?

> I would also suggest splitting this patch in two parts: one moving the
> suffixes array out of dd, and the second one implementing truncate.

Will do.

Best regards,
Ari Sundholm
[email protected]

_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to