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

read through the .h file #278

Open
wants to merge 1 commit into
base: refactor/c-api
Choose a base branch
from
Open

Conversation

diegonehab
Copy link
Contributor

Just read and wrote comments on the C api header file. Haven't looked at the code yet.
Here are additional comments.

how do people know where the PMAs for GIO are, since we don't have the defines
anymore?
    New PMA enums in C API, right?

I think the sibling hashes order used to match the one in solidity. Does the one
in json also matching that?

Flash drive start address configuration is non mandatory anymore (it will be
automatically chosen).
    Do you mean this happens inside the C++ code?
    So people writing binds don't have to replicate the logic?

Removed clua-htif.cpp (its constants are exposed along with other Lua constants
now).
    You mean C API header?

should we rename cm_csr to cm_xfcsr?
    get_xfcsr_address
    read_xfcsr
    write_xfcsr

should functions return cm_error instead of int32_t?

cm_get_proof
/// \details If the node is smaller than a page size,
/// then it must lie entirely inside the same PMA range.
  PMAs have a size that is a multiple of a page size.
  Every node must be aligned to its own size.
  There is no way for an aligned node smaller than a page size to straddle two pages...


The distinction between delete and destroy is kind of weird, right?
Delete calls the i-virtual machine delete.
i-virtual-machien owns the local machine, but does not own the remote machine.
So calling delete on a local machine will really kill it and free all memory.
But calling delete on a remote machine will only free the client-side of things.
Is there a way to improve this?
Maybe different names can capture this idea better?

cm_read_virtual_memory
cm_write_virtual_memory
    After conversion to physical memory, does the entire chunk need to fit
inside the same PMA and does the PMA have to be a memory PMA?


cm_get_root_hash
cm_store
/// \details The method changes machine because it updates the root hash.

    I think we should use a const cast here. This is a "logical const" vs "bitwise
    const" situation.
    All these functions are marked const in C++ in machine.h.
    I thought we had marked (and had to mark) them mutable as well.
    Am I getting crazy?


What happens when we replace a memory range without passing an image_filename
but say it should be shared?

Added cm_get_last_error_message/cm_jsonrpc_get_last_error_message, returning the
error message for the most recent failed call.
    There is no jsonrpc version, right?

It uses a thread local variable, so it's safe to call from different
threads.
    It is safe but it is useless, right? Because you won't get the message... :)

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

Successfully merging this pull request may close these issues.

1 participant