mp_export: Addition of buffer size and written bytes #355

czurnieden · 2019-09-27T03:34:24Z

First sketch.

bn_mp_export.c

minad · 2019-09-30T21:24:27Z

See my comment about written. Furthermore if we really want to improve this function, we should add enums for order and endian. How can we handle the deprecation?

czurnieden · 2019-09-30T22:38:10Z

we should add enums for order and endian.

Yepp, would look better.

How can we handle the deprecation?

Good question. The way bn_deprecated.c works it would need a different function-name, so: rename?

sjaeckel · 2019-10-01T08:11:50Z

How can we handle the deprecation?

Good question. The way bn_deprecated.c works it would need a different function-name, so: rename?

I first thought about renaming it to mp_{ex,im}port_gmp(), but that'd be lying as the current implementation is exactly that (besides the parts where it isn't...) ... this would have been a good name in the first place for those functions... probably mp_{ex,im}port_safe() ?
Or maybe mp_{ex,im}port_gmp_style() or mp_{ex,im}port_gmp_compat()?

I also already thought about having one of those replacements for 1.2 and after 1.2 is out change the API of mp_{ex,im}port()? Would that be acceptable?

minad · 2019-10-01T08:39:01Z

Hmm, I would rather introduce new names. For example mp_pack/mp_unpack?

minad · 2019-10-01T08:42:36Z

mp_to_packed/mp_from_packed would be more consistent with the from_to_bin functions?

sjaeckel · 2019-10-01T08:50:17Z

For example mp_pack/mp_unpack?

it seems you're also one of the goto guys if one has problems with the second of the two hard problems of CS ;-)

mp_to_packed/mp_from_packed would be more consistent with the from_to_bin functions?

I like the first better... they sound natural, whereas those sound pushed to be consistent...

czurnieden · 2019-10-01T15:08:35Z

mp_pack/mp_unpack

Yepp, sounds good.
Do it?

tommath.h

minad · 2019-10-02T19:57:30Z

What's the status here? Ready?

czurnieden · 2019-10-02T21:18:08Z

What's the status here? Ready?

If you are OK with it, drop me a note and I will squash&finish.

minad · 2019-10-03T05:21:06Z

demo/test.c

+      goto LTM_ERR;
+   }
+
+   free(buf);


Free or mp_free? Is this consistent in the test suite

Is this consistent in the test suite
(I'll take it as a question) The rest of the tests is done with static buffers didn't want to do it here, although it is possible.

minad · 2019-10-03T05:22:53Z

There is an endian==0 comparison

minad · 2019-10-03T05:24:42Z

Countp should probably be set to the written count? Maybe add a function mp_pack_size which does this mp_count_bits?

minad · 2019-10-03T05:25:55Z

An internal helper function for the native endian test woul be nice. Are there more functions which check endianness?

doc/bn.tex

demo/test.c

czurnieden · 2019-10-03T15:33:22Z

Countp should probably be set to the written count? Maybe add a function mp_pack_size which does this mp_count_bits?

We have written to mean the size of amount of bytes throughout for now, countp is different in that regard.

But I do support the proposal to put size = (size_t)(((mp_count_bits(&a) + 1) + 7) / 8); into an extra function and I don't know why I haven't done it already.

bn_s_mp_check_endianness.c

tommath.h

bn_s_mp_check_endianness.c

minad

Sorry for me being unclear and complaining too much. I think MP_GET_ENDIAN should be a macro/inline function in tommath_private since it will get optimized away completely.

minad · 2019-10-03T16:12:23Z

Regarding countp - since we added the maxsize we introduced an additional behavior. In that case we want countp to return the actually written units. This mirrors the behavior of the other functions with written argument.

czurnieden · 2019-10-03T16:41:54Z

I think MP_GET_ENDIAN should be a macro/inline function in tommath_private since it will get optimized away completely

That's what I started with but than I read that you wanted a function.
*grr*!
;-)

tommath_private.h

minad

There is still unknown_endian in the docs. I would also like for countp to mirror the behavior of written. But besides that it looks good. Also squash and rebase.

bn_mp_pack.c

bn_mp_pack_size.c

minad · 2019-10-05T19:12:56Z

bn_mp_pack.c

   nail_bytes = nails / 8u;

   bits = (size_t)mp_count_bits(&t);
   count = (bits / ((size * 8u) - nails)) + (((bits % ((size * 8u) - nails)) != 0u) ? 1u : 0u);


Use mp_pack_size/count here?

Use mp_pack_size/count here?

Yes, of course.

So you are OK with what I did?

Yes, but we still have to decide bytes or units.

Right now countp in units, maxsize in bytes, pack_size in bytes? The same applies to mp_unpack.

I shuffled it a bit around, probably more to what you wanted in the first place.
I hope ;-)

Argh, documentation not yet updated!

Yes, but we still have to decide bytes or units.

Do you want to name them (enum)?
Otherwise the types can stay as they are now.

Hmm I don't like it like this. We have count for unpack which is used to pass in the buffer size and for pack we have both maxsize and countp. Either we also change countp/count to bytes or we change maxsize to units. Mixing the units is not good as we have it now.

(I hope I'm up-to-date now with the rebasing)

Hmm I don't like it like this.

Yeah, it's a bit of a mess, admitted.

Either we also change countp/count to bytes or we change maxsize to units.

maxsize (I think it's maxlen elsewhere but I always mix them up) is in all other cases meant to measure the given buffer-size in bytes, a size_t type.

Let me restrict myself to mp_pack for now:
countp does, as not only the name suggests, some (ac)counting. The size in bytes behind countp depends on a second variable (it formerly returned count which were units computed from bitsize and nails) and can be different from the amount of actually used up bytes. so I would say that the expectation is until now that countp returns units.
You won't have a port to get the amount of written bytes if you use countp for units.

On the other side: we have an external function to compute countp so the information thatcountp was once needed for is now available externally and in advance, countp is now free for other uses, e.g.: returning the number bytes written.
It changes the API but it is not much work to write a little macro as a wrapper if so desired.

So for mp_pack I'd propose:

countp (maybe change the name) are the bytes written

maxsize is the size in bytes of the buffer given

count as computed by mp_pack_count returns the number of units you get with the combination of bit-size and nails given to mp_pack_count. That means that there are no accounts of units send in or out of mp_pack.

And for mp_unpack

count is not the number of bytes but the number of units, the actual byte-size of the input is only of internal interest but can be computed from count and nails if so desired although I would just measure the input.

pack/unpack should also be symmetric. This means count should be bytes/unit for both. However we should also consider that changing from units/bytes will introduce errors for user who do not expect that change when they move to the new function.

My suggestion would therefore be to call it mp_pack_count, count counts unit, maxcount etc. This also makes sense. The user is not operating on byte arrays but on arrays of unit size (like calloc for example). The only downside is that its different from the sbin/ubin functions which operate on bytes.

minad · 2019-10-07T15:31:46Z

@sjaeckel what is your opinion on the bytes vs size-units discussion?

sjaeckel · 2019-10-07T16:34:00Z

@sjaeckel what is your opinion on the bytes vs size-units discussion?

I would say we keep the semantics as it was.

i.e. the proposed API should change like that:

diff --git a/tommath.h b/tommath.h
index 2a37709..6b627ff 100644
--- a/tommath.h
+++ b/tommath.h
@@ -384,3 +384,3 @@ size_t mp_pack_count(mp_int *a, size_t nails, size_t size) MP_WUR;
 mp_err mp_pack(void *rop, size_t *countp, mp_order order, size_t size, mp_endian endian,
-               size_t nails, const mp_int *op, size_t maxsize) MP_WUR;
+               size_t nails, const mp_int *op, size_t maxcount) MP_WUR;

mp_pack related API's work with count's of elements, buffers have to be count * size

IMO this makes sense as: a call to mp_pack() with MP_MSB_FIRST, MP_BIG_ENDIAN, nails=0 and size=1 is equal to a call of mp_to_ubin(), in the result which is written out and in the number of written elements ;-)

minad · 2019-10-07T16:35:49Z

I agree with @sjaeckel and I would like to add that mp_pack_count should also return a count. Not a size in bytes.

minad · 2019-10-07T16:38:02Z

Maybe also rearrange the args. void*buf,size_t countmax,size_t*countp

sjaeckel · 2019-10-07T16:44:01Z

I would like to add that mp_pack_count should also return a count

that's what it does already.

Maybe also rearrange the args.

there's no real style how ltm treats the order of args, so I'm somehow against reordering only here, but it's true... somehow it'd also make sense to order them like that mp_err mp_pack(void *rop, size_t maxsize, size_t *countp, ...)

sjaeckel · 2019-10-07T18:31:40Z

mp_err mp_pack(void *rop, size_t maxsize, size_t *countp, ...)

I just thought about it a bit more and yes, please re-order

minad · 2019-10-08T19:56:08Z

superseded by #361

minad changed the title ~~Addition of buffer size and written bytes~~ mp_export: Addition of buffer size and written bytes Sep 30, 2019

minad reviewed Sep 30, 2019

View reviewed changes

bn_mp_export.c Outdated Show resolved Hide resolved

czurnieden force-pushed the refactor_import_export branch from 26a1dcb to 98eb1eb Compare October 1, 2019 14:33

minad added this to the v1.2.0 milestone Oct 1, 2019

minad reviewed Oct 1, 2019

View reviewed changes

tommath.h Outdated Show resolved Hide resolved

czurnieden force-pushed the refactor_import_export branch from aec531d to ede14c7 Compare October 2, 2019 21:24