Add documentation of mambo API used in exercise 2 #118

mskordal · 2024-03-13T18:31:29Z

The following functions are documented:

In api/plugin_support.h:

mambo_alloc
mambo_free
mambo_set_plugin_data
mambo_get_plugin_data
mambo_set_thread_plugin_data
mambo_get_thread_plugin_data
mambo_get_source_addr
mambo_get_thread_id
get_symbol_info_by_addr

In api/hash_table.h:

mambo_ht_init
mambo_ht_add
mambo_ht_get

IgWod

Thank you for the PR. Great work!

I added some comments. Mostly they're minor suggestions to improve readability and add some extra information. However, mambo_set/ger_*_data needs a bit more work. At the moment text in all 4 functions is too confusing. I added some comments, but I'm happy to discuss it further.

IgWod · 2024-03-19T11:36:27Z

api/hash_table.h

+ * @param ht The hash-table to be initialised.
+ * @param initial_size The initial size of the hash-table. Will be round up to a
+ * power of 2.
+ * @param index_shift How many times a key is shifted to determine the index in


I think it's better to say: The number of bits a key ... . For me how many times implies, there are multiple shift operations.

IgWod · 2024-03-19T11:38:10Z

api/hash_table.h

+ *
+ * @param ht The hash-table to add the key-value pair.
+ * @param key The key of the key-value pair.
+ * @param value The value of the key-value pair.


Should we make clear that the value can be a pointer to a more complex data structure, so that the user knows the hashmp supports more than just a 64-bit integer pair?

I mean, a developer can use a variable however they want. As far as the hash-table is conserned, the value is just a value though. I can add that statement, but I believe the function's documentation should focus on what the function does

IgWod · 2024-03-19T11:39:11Z

api/hash_table.h

+ * @pre @c key must be greater than 0.
+ *
+ * @param ht The hash-table to add the key-value pair.
+ * @param key The key of the key-value pair.


We could also say that the key often is an address in the hosted applications that we want to associate the data with.

Again this refers to a use case. We could write a tutorial where we showcase the hash-table being used for that reason. But regarding the hash-table, the key is just a key. I can add it though if we want to add further use cases into the documentation

IgWod · 2024-03-19T11:41:11Z

api/hash_table.h

+ *
+ * @param ht The hash-table to retrieve the value from.
+ * @param key The key used to locate the value within the hash-table.
+ * @param value Pointer to the address where the value will be stored after this


What about: "Pointer where the address of the retrieved data will be stored after this..."? I know it's a tricky one to write.

IgWod · 2024-03-19T11:42:16Z

api/plugin_support.h

+ * DBM use case.
+ *
+ * An mmap wrapper that allocates private to MAMBO memory space with read/write
+ * permissions.


Let's also add: It should be used instead of the regular libc malloc inside plugins.

api/plugin_support.h

IgWod · 2024-03-27T21:29:47Z

api/plugin_support.h

+ * accessible between callbacks of all threads running in MAMBO.
+ *
+ * @param ctx The context that holds the plugin state.
+ * @param data Pointer to the address where data will be stored.


Again this is very confusing. Do you mean: Pointer that is stored by the helpers and that pointer points to a global data structure? I think now the text implies that data is the pointer where the helper will store some data.

IgWod · 2024-03-27T21:32:24Z

api/plugin_support.h

+ *
+ * @param addr The untranslated address of the application's instruction.
+ * @param sym_name Pointer to the pointer to the address where the symbol name
+ * will be stored.


Or simply: Pointer to a string?

IgWod · 2024-03-27T21:32:48Z

api/plugin_support.h

+ * will be stored.
+ * @param start_addr Pointer to the pointer to the address where the starting
+ * address of the symbol name will be stored.
+ * @param filename Pointer to the pointer to the address where the filename will


Again: Pointer to a string?

IgWod · 2024-03-27T21:34:06Z

api/plugin_support.h

+ * @param addr The untranslated address of the application's instruction.
+ * @param sym_name Pointer to the pointer to the address where the symbol name
+ * will be stored.
+ * @param start_addr Pointer to the pointer to the address where the starting


I'd simplify this one to: Pointer to a pointer where the starting ...

I know we discussed that, but the current writing makes it confusing when I'm reading it.

IgWod

Thank you for addressing the feedback. The PR is almost ready, but I added few small comments to remove any potential confusion from the text.

IgWod · 2024-05-08T13:13:51Z

api/plugin_support.h

 void mambo_free(mambo_context *ctx, void *ptr);

 /* Access plugin data */
+/**
+ * @brief Stores an address to a global structure, making data of that address


What about: ... making data stored at that address ... ?

IgWod · 2024-05-08T13:16:28Z

api/plugin_support.h

+ * accessible between callbacks of all threads running in MAMBO.
+ *
+ * @param ctx The context that holds the plugin state.
+ * @param data The pointer of the address to be stored.


Should be either: "The address to be stored" or "The pointer to be stored" or ".... pointer to the address ...". Otherwise it'd imply data is pointer to pointer. In this case think about void* as uintptr_t.

IgWod · 2024-05-08T13:19:25Z

api/plugin_support.h

+
+/**
+ * @brief Uses the untranslated address of the application's instruction to
+ * return the symbol name, the starting address of the symbol name and the


Remove "name" in "the starting address of the symbol name".

IgWod

Other than the small typo it looks good to me. I added @jkressel so he can review it as well.

IgWod · 2024-05-29T10:46:16Z

api/plugin_support.h

 void mambo_free(mambo_context *ctx, void *ptr);

 /* Access plugin data */
+/**
+ * @brief Stores an address to a global structure, making data storedat that


A small typo: "storedat" -> "stored at"

The following functions are documented: --------------------------------------- In api/plugin_support.h: ------------------------ mambo_alloc mambo_free mambo_set_plugin_data mambo_get_plugin_data mambo_set_thread_plugin_data mambo_get_thread_plugin_data mambo_get_source_addr mambo_get_thread_id get_symbol_info_by_addr In api/hash_table.h: -------------------- mambo_ht_init mambo_ht_add mambo_ht_get

IgWod self-assigned this Mar 14, 2024

IgWod self-requested a review March 14, 2024 09:50

IgWod requested changes Mar 27, 2024

View reviewed changes

mskordal force-pushed the doc_branch branch 2 times, most recently from d10aec5 to c37af0f Compare April 12, 2024 11:53

IgWod requested changes May 8, 2024

View reviewed changes

mskordal force-pushed the doc_branch branch from c37af0f to ade689b Compare May 26, 2024 10:08

IgWod reviewed May 29, 2024

View reviewed changes

IgWod requested a review from jkressel May 29, 2024 10:48

mskordal force-pushed the doc_branch branch from ade689b to 4b1136f Compare May 30, 2024 08:18

mskordal force-pushed the doc_branch branch from 4b1136f to 7ef160d Compare June 2, 2024 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation of mambo API used in exercise 2 #118

Add documentation of mambo API used in exercise 2 #118

mskordal commented Mar 13, 2024

IgWod left a comment

IgWod Mar 19, 2024

IgWod Mar 19, 2024

mskordal Apr 12, 2024

IgWod Mar 19, 2024

mskordal Apr 12, 2024

IgWod Mar 19, 2024

IgWod Mar 19, 2024

IgWod Mar 27, 2024

IgWod Mar 27, 2024

IgWod Mar 27, 2024

IgWod Mar 27, 2024

IgWod left a comment

IgWod May 8, 2024

IgWod May 8, 2024

IgWod May 8, 2024

IgWod left a comment

IgWod May 29, 2024

Add documentation of mambo API used in exercise 2 #118

Are you sure you want to change the base?

Add documentation of mambo API used in exercise 2 #118

Conversation

mskordal commented Mar 13, 2024

The following functions are documented:

In api/plugin_support.h:

In api/hash_table.h:

IgWod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgWod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgWod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment