Re: [PATCH] Add bc (Last patch)
On Mon, Oct 29, 2018 at 4:46 PM Denys Vlasenko wrote:
>
> On Mon, Oct 29, 2018 at 4:55 PM Gavin Howard wrote:
> >
> > Hello,
> >
> > Because my patch is so large, I cannot get it through the mailing list
> > filter. Therefore, I have pasted it on pastebin, instead.
> >
> > The link is https://pastebin.com/PJa2vaUR and the raw version can be
> > found at https://pastebin.com/raw/PJa2vaUR
>
> Probably could send it bzipped.
I tried, unfortunately. Even though it was under the limit, the
mailing list software rejected.
> All functions except bc_main() should be static.
> Check with nm --size-sort bc.o
I will see what I can do about that.
> Use : the entire
>
> +BcStatus bc_read_file(const char *path, char **buf) {
> +
> +BcStatus s = BC_STATUS_IO_ERR;
> +FILE *f;
> +size_t size, read;
> +long res;
> +struct stat pstat;
> +
> +if (!(f = fopen(path, "r"))) return BC_STATUS_EXEC_FILE_ERR;
> +
> +if (fstat(fileno(f), ) == -1) goto malloc_err;
> +
> +if (S_ISDIR(pstat.st_mode)) {
> +s = BC_STATUS_PATH_IS_DIR;
> +goto malloc_err;
> +}
> +
> +if (fseek(f, 0, SEEK_END) == -1) goto malloc_err;
> +if ((res = ftell(f)) < 0) goto malloc_err;
> +if (fseek(f, 0, SEEK_SET) == -1) goto malloc_err;
> +
> +if (!(*buf = malloc((size = (size_t) res) + 1))) {
> +s = BC_STATUS_ALLOC_ERR;
> +goto malloc_err;
> +}
> +
> +if ((read = fread(*buf, 1, size, f)) != size) goto read_err;
>
> can be replaced by single xmalloc_open_read_close()!
Will do.
> (The entire
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> should be replaced by
> #include "libbb.h"
> #include
> )
Sorry about that. I tried to follow the documentation as exactly as I could.
> +while ((c = getopt_long(argc, argv, bc_args_opt, bc_args_lopt,
> )) != -1)
> +{
>
> Use getopt32() ?
Okay. Will do.
> +typedef struct BcResult {
> +
> +BcResultType t;
> +BcResultData d;
> +
> +} BcResult;
>
> Lets drop these extra empty lines.
Will do.
> +#ifdef CONFIG_BC
> +BcStatus bc_vm_posixError(BcStatus s, const char *file,
> + size_t line, const char *msg);
> +#endif // CONFIG_BC
>
> "ifdef CONFIG_foo" is unsafe versus typos. Use "if ENABLE_foo" - it
> gives warnings
> for typos.
Oh? Did not know that.
> +#ifdef CONFIG_BC
> +
> +#endif // CONFIG_BC
>
> Why these empty ifdef blocks?
This was caused by bugs in my release script that have been fixed.
> +const char bc_name[] = "bc";
>
> static
>
>
> +const char *bc_errs[] = {
>
> static const char *const bc_errs[] = {
Will do.
> +if (!(v->v = malloc(esize * BC_VEC_START_CAP))) return
> BC_STATUS_ALLOC_ERR;
>
> (1) Please do not nest assignments:
>
> v->v = malloc(esize * BC_VEC_START_CAP);
> if (!v->v)
> return BC_STATUS_ALLOC_ERR;
Oh boy. I was afraid of this.
The problem is that I am also trying to get this bc into toybox (might
as well reduce duplication of effort), and as I am sure you know,
Landley is very particular about the line count of commands. To be
honest, I would prefer your style (except for putting if statements
that on one line if possible), but this reduces line count.
I don't really know what to do here.
> (2) Use xmalloc/xzalloc/xrealloc which never return NULL.
> We do not try to survive out-of-memory. No check is necessary.
Will do. I was not aware of those functions. bc does have to print a
diagnostic message (see
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/bc.html#tag_20_09_15),
but I checked that those procedures do that.
> +BcStatus bc_vec_pushByte(BcVec *v, uint8_t data) {
> +return bc_vec_push(v, );
> +}
>
> The entire bbox code base uses this formatting of function body:
>
> BcStatus bc_vec_pushByte(BcVec *v, uint8_t data)
> {
> return bc_vec_push(v, );
> }
This one might be a little harder.
> +if (!v->len && (s = bc_vec_pushByte(v, '\0'))) return s;
>
> len is not boolean or a pointer. !foo is usually used with those.
> len is an integer. You are testing whether it's zero. Why obfuscate?
> For readability, should be:
>if (v->len == 0) {
>s = bc_vec_pushByte(v, '\0');
>if (s)
>return s;
> }
>
>
> +if (a == b) return 0;
> +else if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
> +else if (!b->len) return BC_NUM_NEG(1, a->neg);
> +else if (a->neg) {
> +if (b->neg) neg = true;
> +else return -1;
> +}
> +else if (b->neg) return 1;
>
> "else" after exiting code in preceding "if" is not necessary.
> Can rewrite as:
>
> if (a == b) return 0;
> if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
> if (!b->len) return BC_NUM_NEG(1, a->neg);
> if (a->neg) {
> if (!b->neg) return -1;
> neg = true;
> }
> else if (b->neg) return 1;
You are correct.
>
Re: [PATCH] Add bc (Last patch)
On Mon, Oct 29, 2018 at 4:55 PM Gavin Howard wrote:
>
> Hello,
>
> Because my patch is so large, I cannot get it through the mailing list
> filter. Therefore, I have pasted it on pastebin, instead.
>
> The link is https://pastebin.com/PJa2vaUR and the raw version can be
> found at https://pastebin.com/raw/PJa2vaUR
Probably could send it bzipped.
All functions except bc_main() should be static.
Check with nm --size-sort bc.o
Use : the entire
+BcStatus bc_read_file(const char *path, char **buf) {
+
+BcStatus s = BC_STATUS_IO_ERR;
+FILE *f;
+size_t size, read;
+long res;
+struct stat pstat;
+
+if (!(f = fopen(path, "r"))) return BC_STATUS_EXEC_FILE_ERR;
+
+if (fstat(fileno(f), ) == -1) goto malloc_err;
+
+if (S_ISDIR(pstat.st_mode)) {
+s = BC_STATUS_PATH_IS_DIR;
+goto malloc_err;
+}
+
+if (fseek(f, 0, SEEK_END) == -1) goto malloc_err;
+if ((res = ftell(f)) < 0) goto malloc_err;
+if (fseek(f, 0, SEEK_SET) == -1) goto malloc_err;
+
+if (!(*buf = malloc((size = (size_t) res) + 1))) {
+s = BC_STATUS_ALLOC_ERR;
+goto malloc_err;
+}
+
+if ((read = fread(*buf, 1, size, f)) != size) goto read_err;
can be replaced by single xmalloc_open_read_close()!
(The entire
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
should be replaced by
#include "libbb.h"
#include
)
+while ((c = getopt_long(argc, argv, bc_args_opt, bc_args_lopt,
)) != -1)
+{
Use getopt32() ?
+typedef struct BcResult {
+
+BcResultType t;
+BcResultData d;
+
+} BcResult;
Lets drop these extra empty lines.
+#ifdef CONFIG_BC
+BcStatus bc_vm_posixError(BcStatus s, const char *file,
+ size_t line, const char *msg);
+#endif // CONFIG_BC
"ifdef CONFIG_foo" is unsafe versus typos. Use "if ENABLE_foo" - it
gives warnings
for typos.
+#ifdef CONFIG_BC
+
+#endif // CONFIG_BC
Why these empty ifdef blocks?
+const char bc_name[] = "bc";
static
+const char *bc_errs[] = {
static const char *const bc_errs[] = {
+if (!(v->v = malloc(esize * BC_VEC_START_CAP))) return BC_STATUS_ALLOC_ERR;
(1) Please do not nest assignments:
v->v = malloc(esize * BC_VEC_START_CAP);
if (!v->v)
return BC_STATUS_ALLOC_ERR;
(2) Use xmalloc/xzalloc/xrealloc which never return NULL.
We do not try to survive out-of-memory. No check is necessary.
+BcStatus bc_vec_pushByte(BcVec *v, uint8_t data) {
+return bc_vec_push(v, );
+}
The entire bbox code base uses this formatting of function body:
BcStatus bc_vec_pushByte(BcVec *v, uint8_t data)
{
return bc_vec_push(v, );
}
+if (!v->len && (s = bc_vec_pushByte(v, '\0'))) return s;
len is not boolean or a pointer. !foo is usually used with those.
len is an integer. You are testing whether it's zero. Why obfuscate?
For readability, should be:
if (v->len == 0) {
s = bc_vec_pushByte(v, '\0');
if (s)
return s;
}
+if (a == b) return 0;
+else if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
+else if (!b->len) return BC_NUM_NEG(1, a->neg);
+else if (a->neg) {
+if (b->neg) neg = true;
+else return -1;
+}
+else if (b->neg) return 1;
"else" after exiting code in preceding "if" is not necessary.
Can rewrite as:
if (a == b) return 0;
if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
if (!b->len) return BC_NUM_NEG(1, a->neg);
if (a->neg) {
if (!b->neg) return -1;
neg = true;
}
else if (b->neg) return 1;
+static void bc_vm_sig(int sig) {
+if (sig == SIGINT) {
+size_t len = strlen(bcg.sig_msg);
+if (write(2, bcg.sig_msg, len) == (ssize_t) len)
+bcg.sig += (bcg.signe = bcg.sig == bcg.sigc);
+}
+else bcg.sig_other = 1;
+}
+sa.sa_handler = bc_vm_sig;
+sa.sa_flags = 0;
+
+if (sigaction(SIGINT, , NULL) < 0 || sigaction(SIGPIPE, ,
NULL) < 0 ||
+sigaction(SIGHUP, , NULL) < 0 || sigaction(SIGTERM, , NULL) < 0)
+{
+return BC_STATUS_EXEC_SIGACTION_FAIL;
+}
Calculator has signal handling?!
siganction() never fails - no need to check result.
Better yet, use read_line_input() from libbb.h:
/*
* maxsize must be >= 2.
* Returns:
* -1 on read errors or EOF, or on bare Ctrl-D,
* 0 on ctrl-C (the line entered is still returned in 'command'),
* >0 length of input string, including terminating '\n'
*/
int read_line_input(line_input_t *st, const char *prompt, char
*command, int maxsize) FAST_FUNC;
- it already has Ctrl-C handling, and drop entire signal handling mess.
(It's buggy: write() can modify errno).
Let bc just die on SIGPIPE, SIGTERM and SIGHUP.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: bc
Just sent it today, actually. Thank you. Gavin H. On Mon, Oct 29, 2018 at 4:05 PM Denys Vlasenko wrote: > > On Fri, Oct 12, 2018 at 1:11 AM Gavin Howard wrote: > > Hello, > > > > In March, I wrote > > (http://lists.busybox.net/pipermail/busybox/2018-March/086300.html) > > asking if there was interest in having my bc > > (https://github.com/gavinhoward/bc) in busybox. I got some interest > > back then. > > > > Well, it is ready. Do the maintainers still want it? > > No need to ask. Just send the patch for review. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: bc
On Fri, Oct 12, 2018 at 1:11 AM Gavin Howard wrote: > Hello, > > In March, I wrote > (http://lists.busybox.net/pipermail/busybox/2018-March/086300.html) > asking if there was interest in having my bc > (https://github.com/gavinhoward/bc) in busybox. I got some interest > back then. > > Well, it is ready. Do the maintainers still want it? No need to ask. Just send the patch for review. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
rtcwake s5 support
Hello All, Correct me if I am wrong, but the busybox version of rtcwake doesn't seem to support s5(poweroff). Is this something that would be acceptable to add? I can put together a patch to add support. Thanks, Justin ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] nbd-client: support newstyle protocol, -b, -d
On Wed, 24 Oct 2018 15:53:44 +0200 Denys Vlasenko wrote: > I applied a modified version of the patch, > please let me know whether it works. > Thanks! It does, both on x86_64 and MIPS64be, with old and new servers. Thank you! ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] Add bc (Last patch)
Hello, Because my patch is so large, I cannot get it through the mailing list filter. Therefore, I have pasted it on pastebin, instead. The link is https://pastebin.com/PJa2vaUR and the raw version can be found at https://pastebin.com/raw/PJa2vaUR I look forward to working with the maintainers on getting this added. (And sorry for the moderator approval spam.) Gavin Howard ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
