[PATCH] Add bc (Last patch)

Denys Vlasenko vda.linux at googlemail.com
Mon Oct 29 22:46:21 UTC 2018


On Mon, Oct 29, 2018 at 4:55 PM Gavin Howard <gavin.d.howard at gmail.com> 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 <libbb.h>: 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), &pstat) == -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 <ctype.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <fcntl.h>
+#include <limits.h>
+#include <signal.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include <getopt.h>
should be replaced by
#include "libbb.h"
#include <if_you_need_anything_not_already_included_by_libbb.h>
)


+    while ((c = getopt_long(argc, argv, bc_args_opt, bc_args_lopt,
&idx)) != -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, &data);
+}

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, &data);
}


+    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, &sa, NULL) < 0 || sigaction(SIGPIPE, &sa,
NULL) < 0 ||
+        sigaction(SIGHUP, &sa, NULL) < 0 || sigaction(SIGTERM, &sa, 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.


More information about the busybox mailing list