Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate msgpack.ibuf_decode #352

Closed
locker opened this issue Dec 1, 2017 · 12 comments
Closed

Deprecate msgpack.ibuf_decode #352

locker opened this issue Dec 1, 2017 · 12 comments
Assignees

Comments

@locker
Copy link
Member

locker commented Dec 1, 2017

Mark msgpack.ibuf_decode as deprecated. Recommend using msgpack.decode or msgpack.decode_unchecked instead.

See: tarantool/tarantool#2755

@locker locker added the 1.8 label Dec 1, 2017
@lenkis lenkis self-assigned this Dec 4, 2017
@lenkis
Copy link
Contributor

lenkis commented Dec 11, 2017

This should go to both 1.7 and 1.8 doc branches.
Peter, please add to 1.7, and I'll merge it further to 1.8.

@lenkis lenkis removed their assignment Dec 11, 2017
@lenkis lenkis removed the 1.8 label Dec 11, 2017
lenkis added a commit that referenced this issue Dec 18, 2017
Fixes gh-352 Deprecate msgpack.ibuf_decode
@Totktonada
Copy link
Member

The new function variants that accept a string buffer were introduced in tarantool/tarantool#2755: that is the reason of deprecation of the separate msgpack.ibuf_decode function. The new variants should be described in the website documentation. Now the documentation lacks it.

The linked issue has great formal description of the new API. It would be good to have it on the website.

@Totktonada Totktonada reopened this Nov 5, 2019
@pgulutzan
Copy link
Contributor

The 1.10 documentation lacks description of some functions that are in the 2.2 documentation; Are those the functions that are supposed to be described? If so, perhaps long ago I pushed a change to the wrong version.

@pgulutzan pgulutzan added the need feedback [special status] On hold, awaiting feedback label Dec 3, 2019
@pgulutzan pgulutzan removed the need feedback [special status] On hold, awaiting feedback label Mar 26, 2020
@pgulutzan
Copy link
Contributor

There was no reply, and the changes have been copied to the 1.10 documentation, so I have removed the "need feedback" tag and am closing this issue.

@Totktonada
Copy link
Member

Sorry for reopening this again, but encode / decode / decode_unchecked variants, which reads from or writes to a char * buffer, are not described in the docs. See tarantool/tarantool@68c2139

@Totktonada Totktonada reopened this May 28, 2020
@pgulutzan
Copy link
Contributor

I looked at the version-2.4 decode_unchecked documentation, here:
https://www.tarantool.io/en/doc/2.4/reference/reference_lua/msgpack/#msgpack-decode-unchecked
I clicked on the word "buffer" in the sentence "For an example see the buffer module."
I ran the example there, which includes decode_unchecked.
The result is what the manual describes (at least in 2.4).
So I am trying to understand why, although it is described in the docs, it is not found.
Perhaps my mistake is that I did not properly link, or did not properly explain when I closed?

@Totktonada
Copy link
Member

Several points:

  • encode() may accept a buffer (buffer.ibuf() kind) as the second argument. Note also that it returns number of written bytes in the case, not a Lua string with encoded msgpack data.

  • decode() may accept a buffer (cdata<char *> kind) as the first argument and it also changes meaning of the second argument (it becomes the buffer size).

  • Re decode_unchecked(): cdata<char *> or cdata<const char *> is not string. They are different types in Lua. So current description can be confusing for users.

  • Cite:

    Because checking is skipped, decode_unchecked() can operate with string pointers to buffers which decode() cannot handle.

    I would not say that support of char * is consequence of lack of buffer bounds checks.

@Totktonada
Copy link
Member

(@pgulutzan Feel free to ask me details here or privately if something is not clear or if you think that I missed something.)

@pgulutzan
Copy link
Contributor

@Toktonada: Thank you. I made several changes to the version 2.4 manual, but won't close if you think it is still not done.

@Totktonada
Copy link
Member

.. function:: decode_unchecked(C_style_string_pointer[, size])
Input and output are the same as for
:ref:decode(C_style_string_pointer) <msgpack-decode_c_style_string_pointer>,
except that size is optional.

decode_unchecked() with C-style-pointed (cdata first argument) does not check size at all (even if it passed). I think we should not see optional size in the function arguments.

How to verify:

tarantool> msgpack = require('msgpack')
tarantool> ffi = require('ffi')
tarantool> msgpack.decode(ffi.cast('char *', '\x91\x00'), 1)
---
- error: 'msgpack.decode: invalid MsgPack'
...

tarantool> msgpack.decode_unchecked(ffi.cast('char *', '\x91\x00'), 1)
---
- [0]
- 'cdata<char *>: 0x40eb9d82'
...

Except this, I don't have objections. Please, update all versions and close the issue finally.

pgulutzan added a commit that referenced this issue Jun 2, 2020
(cherry picked from commit 2cea32a)
@pgulutzan
Copy link
Contributor

Okay, changes pushed in 2.4, 2.3, 2.2, and 1.10.

@Totktonada
Copy link
Member

Thank you, Peter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants