[PATCH] bc Version 1.1

[Denys Vlasenko]

On Sun, Dec 2, 2018 at 2:25 AM Gavin Howard <gavin.d.howard at gmail.com> wrote:
>
> I have an updated patch for the comments: https://pastebin.com/ZCvPitQ0
>
> Raw is at: https://pastebin.com/raw/ZCvPitQ0
>
> > Sorry for the delay in reviewing.
> >
> > Let's take a look at bc_vm_printf(). Basically:
> >         bad = vfprintf(f, fmt, args) < 0;
> >         if (bad) bc_vm_exit(BC_STATUS_IO_ERR);
> >
> > What does bc_vm_exit() do? -
> > static void bc_vm_exit(BcStatus s) {
> >         bc_vm_printf(stderr, bc_err_fmt, bc_errs[bc_err_ids[s]],
> > bc_err_msgs[s]);
> >         exit((int) s);
> > }
> >
> > This would enter infinite loop if you bc_vm_printf(stderr) and meet
> > IO error.
>
> Agreed. That line is hard to test. Sorry that I missed that obvious
> mistake when adding xmalloc support. It has been fixed in the patch in
> this email.
>
> > > $ $BC_ROOT/tests/all.sh bc $BUSYBOX_ROOT/busybox bc
> >
> > Trying.... does this output means tests failed or succeeded?? -
>
> Succeeded. The important bit is what the return value ("$?") is. If it
> returned 0, no errors. Non-zero: an error.
>
> The reason for this is because the test suite also tests that the bc
> will not crash when given invalid input. Obviously, error messages
> will be printed in that case, but it means that the bc is working
> well.
>
> > In fact, all calls of bc_vm_exit() are called with
> > s == BC_STATUS_IO_ERR.
> >
> > Thus, bc_errs[bc_err_ids[s]], bc_err_msgs[s] are constants.
>
> Changed. Be aware that I still used the arrays because they are used
> for other statuses, so it was better to keep the strings in the
> arrays.
>
> By the way, the original reason it was this way was because in order
> to call xmalloc and friends, I had to implement versions for the
> non-busybox bc, and they call bc_vm_exit as well, making the parameter
> necessary in the standalone version of bc. However, I couldn't find
> analogous busybox versions of bc_vm_printf and friends, so I had to
> keep those in the busybox version. In the original patch, I just
> didn't remove the parameters.
>
> > static void bc_vm_info(const char *const help)
> > {
> >         printf("%s "BB_VER"\n", applet_name);
> >         fputs(bc_copyright, stdout);
> >         if (help) printf(help, applet_name);
> > }
> >
> > In all callsites, help is NULL.
>
> Changed in the patch to your BB_VER print statement, and the help is removed.
>
> >        return s == BC_STATUS_QUIT ? BC_STATUS_SUCCESS : s;
> >
> > You can write it in more straightforward way:
> >
> >        if (s == BC_STATUS_QUIT)
> >                s = BC_STATUS_SUCCESS;
> >        return s;
>
> I must admit that I disagree, but it is changed in the patch.
>
> To make things easier on you for reviewing the changes in the patch,
> which do include changes that I made myself in the time between my
> last submission and now, I have added a patch below to show the
> changes from the previous version. This patch is not valid; it's only
> for your convenience.

I'm currently working on the code you sent me. I'll
post my updated version (or push it to git) later.

It might be a good idea for you to wait for that to happen
before sending patches.

Current observations: bc_args() never fails, no need to return status.
Therefore, bc_vm_envArgs() never fails, no need to return status.
Therefore, bc_vm_init() never fails, no need to return status.
Therefore, in bc_vm_run() jumps to "exit:" never happen.