make mp_to_radix return the count of characters of the converted number

[czurnieden]

As suggested by @sjaeckel in #343

Mmh…

[czurnieden]

First problem is, as you can see, are the macros but that was expected and is, I think, solvable.
The problms with the MS-compiler are…interresting.

[czurnieden]

I find it a bit awkward that way (there are alternatives which I find equally or even more odd, but YMMV, so drop a note, please).

A cleaner way, I think, would be to add another parameter, maybe

mp_err mp_to_radix(const mp_int *a, char *str, size_t maxlen, size_t *used_length, int radix)

or something in that line.

[sjaeckel]

I find it a bit awkward that way

what do you find awkward?

What do you think about replacing mp_radix_size() in favor of mp_to_radix()?

• len = 0; mp_to_radix(x, NULL, &len, radix); where len will then be filled with the approximated-to-be-allocated length... this wouldn't even need a separate error code...
• len = 4567; mp_to_radix(x, buf, &len, radix); (where 4567 is enough) and len get's decreased to 4566 because the approximation overshot
• an option would be len = 123; mp_to_radix(x, buf, &len, radix); (where 123 is too small to hold x) we would need a separate error code to tell the user to increase the buffer size

[minad]

Removing radix_size is a good idea. We could still branch to an O(1) size computation function internally if buf==NULL.

[minad]

The function in #343 could be used internally.

[minad]

@sjaeckel what shall we do about the to_signed_bin functions? They also need a size argument. These functions should probably be treated differently since size computation is cheaper. But I don't like the difference in API now.

[sjaeckel]

The function in #343 could be used internally.

I'd say either we drop the original one or we keep them both through a compile-time setting if someone doesn't want to spend the space for the tables

@sjaeckel what shall we do about the to_signed_bin functions? They also need a size argument. These functions should probably be treated differently since size computation is cheaper. But I don't like the difference in API now.

very good point! IMO we should deprecate the ones w/o length
... and probably can we then rename mp_to_unsigned_bin_n to mp_to_unsigned_bin for 2.0 (and provide a #define mp_to_unsigned_bin_n mp_to_unsigned_bin) ... not sure though ...

[minad]

Just introduce functions with new names mp_to_ubin and mp_to_sbin?

[czurnieden]

The function in #343 could be used internally.
I'd say either we drop the original one or we keep them both through a compile-time setting if someone doesn't want to spend the space for the tables

Tables has about 14k (depending a bit on MP_xBIT but not much), original about 3k , and the one with mp_ilogb has about 6k.
Oh, I though it would be more!

what do you find awkward?

My social interactions.
But making fun of myself aside:

What do you think about replacing mp_radix_size() in favor of mp_to_radix()?

That would be quite radical aaaaaand I like it! ;-)
*rolls up sleeves*

[minad]

Hmm, I worked a bit on mp_to_radix and I am not sure if deprecating mp_radix_size is the right way.

We have the following scenarios:

• str==NULL, *len=returns required length
• str len too small, *len returns written chars or required length?
• str len large enough, *len returns written chars

In particular the overloading in the case of buffer overflow is not nice. We could always return the needed length, but then the division would have to continue even if no chars are written anymore.
Generally, I think such overloading makes the library harder to use. It is non obvious anymore, by just looking at the functions. I have to check the documentation what the function does in which scenario etc.

My recommendation would be to not even merge this PR and just keep the length argument as value. The user can compute the number of written characters themselves. They know their buffer size and they know mp_radix_size. This doesn't sound userfriendly either, but I don't think this is a common use case. The standard use case is 1. calling mp_radix_size 2. allocating a buffer 2. calling mp_to_radix. The second use case is just calling mp_to_radix with a static buffer and hoping that the buffer is large enough (maybe falling back to allocating if not).

[czurnieden]

Hmm, I worked a bit on mp_to_radix and I am not sure if deprecating mp_radix_size is the right way.

The exact details are debatable, of course, but in general it is a good idea to get rid of it.

Since we got mp_ilogb the function mp_radix_size is reduced to not much more than a convenient little wrapper for mp_ilogb and every user who needs an exact size without actually printing the number can do it themselves, a note in the docs is more than sufficient here.

No to_string function I know needs to get given the size in advance, to the contrary, most of them return the number of characters needed. They only need a pointer and do the allocating themself, only freeing is left to the user. Used internally is in most cases a kind of log-function that overshoots by a couple and the actual buffer gets cut at the end as needed, if at all.
Although we cannot do it exactly in that way, it is not the worst solution, I think.

In particular the overloading in the case of buffer overflow is not nice.

I understood it in the way that that case is meant to replace mp_to_radix_n?
I still think that something like

mp_err mp_to_radix(const mp_int *a, char *str, size_t *length_written, size_t size, int radix) 

with

• str == NULL -> FAIL because you cannot free that buffer after use anymore
• length_written == NULL -> OK (if the caller doesn't want/need to know, who are we to insist?)
• size == 0 -> OK, do the "computing size and allocating buffer" internally
• size != 0 -> OK, do the "allocating buffer" internally and convert only the first size digits output (like the old mp_toradix_n).

would be better.
It would also be quite easy to the the old shortcut macros like mp_todecimal, mp_tohex etc.

@sjaeckel

But I don't like the difference in API now.

Could you be a bit more specific, please?

[minad]

@czurnieden I think a function which allocates would also be acceptable, similar to what gmp offers.

But I don't like the difference in API now.
Could you be a bit more specific, please?

As of now mp_to_unsigned_bit, mp_unsigned_size, mp_to_radix, mp_radix_size etc all have a similar API. We should decide if we want to have the functions consistent. Should they all take buffers and allocate if no buffer is given? What shall they do if the buffer is not large enough?
Will we continue to offer mp_*_size functions?

[minad]

EDIT: See updated proposal below

size_t mp_ubin_size(const mp_int *a) MP_WUR;
mp_err mp_from_ubin(mp_int *a, const unsigned char *buf, size_t size) MP_WUR;
mp_err mp_to_ubin(const mp_int *a, unsigned char *buf, size_t maxsize, size_t* written) MP_WUR;

size_t mp_sbin_size(const mp_int *a) MP_WUR;
mp_err mp_from_sbin(mp_int *a, const unsigned char *buf, size_t size) MP_WUR;
mp_err mp_to_sbin(const mp_int *a, unsigned char *buf, size_t maxsize, size_t* written) MP_WUR;

mp_err mp_radix_size(const mp_int *a, size_t *size, unsigned radix) MP_WUR;
mp_err mp_from_radix(mp_int *a, const char *str, unsigned radix) MP_WUR;
mp_err mp_to_radix(const mp_int *a, char *str, size_t maxsize, size_t* written, unsigned radix) MP_WUR;~

As @czurnieden proposed above, the mp_to_radix, mp_to_sbin and mp_to_ubin functions will allocate when str == NULL && size==0. str != NULL && size == 0 and str == NULL && size != 0 yield MP_VAL errors. Written bytes will be returned if written!=NULL.

[czurnieden]

A quick try at the propositions. To be debated.
(*bin* not ignored, just not touched yet)

There is a realloc for now in mp_to_radix which can get removed if necessary.

[czurnieden]

Took the liberty to add "work in progress" because that's what it is, isn't it?

[sjaeckel]

I'm skeptic about the pattern-change from "alloc outside" to "alloc inside", but I somehow like it. I'd say if we do it like that we won't need the written arg anyways, right?

size_t mp_ubin_size(const mp_int *a) MP_WUR;
mp_err mp_from_ubin(mp_int *a, const unsigned char *buf, size_t size) MP_WUR;
mp_err mp_to_ubin(const mp_int *a, unsigned char **buf, size_t *len) MP_WUR;

size_t mp_sbin_size(const mp_int *a) MP_WUR;
mp_err mp_from_sbin(mp_int *a, const unsigned char *buf, size_t size) MP_WUR;
mp_err mp_to_sbin(const mp_int *a, unsigned char **buf, size_t *len) MP_WUR;

mp_err mp_radix_size(const mp_int *a, size_t *size, unsigned radix) MP_WUR;
mp_err mp_from_radix(mp_int *a, const char *str, unsigned radix) MP_WUR;
mp_err mp_to_radix(const mp_int *a, char **str, size_t *len, unsigned radix) MP_WUR;

where

1. *len is always ignored as input
2. we use the internal size-approximation
   2.a to do either the malloc if *str is NULL
   2.b or realloc *str to that size.
3. update *len with the actual size

[minad]

No realloc! This is a bad idea!

[sjaeckel]

No realloc! This is a bad idea!

then no allocation inside IMO

[sjaeckel]

No realloc! This is a bad idea!

then no allocation inside IMO

we can also do a free + malloc as we don't need the copy

[czurnieden]

Did you manage to get this NOP issue fixed?

Fixed. It was my fault: a typo that I wasn't able to recognize for some reason, despite it—the typo, that is—screaming at me the whole time.
What did I do: well, instead of using the inner part MP_FOOBAR of the defining macro BN_MP_FOOBAR_C I C&P'd it from the end, so I had MP_FOOBAR_C.

Yes, I'm an idiot, sue me ;-)

[minad]

Status? Could you please squash?

[czurnieden]

We are only waiting for your OK, so if you are OK with it, drop me a note and I will squash&finish.
Yes, I C&P'ed ;-)

[minad]

Looks good

[sjaeckel]

LGTM

[sjaeckel]

oh I just realized that it isn't done here... can you please change len to be size_t

[minad]

Squash, rebase and fix CI? But besides that I approve.

[czurnieden]

fix CI?

CI?

[czurnieden]

@minad

fix CI?

Oh, you mean Travis (continuous integration)?
I cannot repair it, I don't work here.

Because I just cannot resist: it got a bit down here with the new owner, didn't it? >;->
*ducks&runs*

[czurnieden]

I cannot repair it, I don't work here.

Ah, no that was over at #348
(I really shouldn't do that many PRs in parallel!)

[minad]

Looks good

[minad]

@czurnieden thanks :) @sjaeckel from my side this looks ready.