Repository navigation
feat: implement sd_bus_read_dict function - #31
Merged
Merged
Conversation
Reviewer's GuideThis PR adds the missing sd_bus_read_dict function to service.c, implementing full D-Bus dictionary parsing into a GHashTable by navigating array and dict entry containers, extracting key/value strings, and performing robust error handling. Class diagram for sd_bus_read_dict additionclassDiagram
class sd_bus_message {
+enter_container(type, signature)
+exit_container()
+read_basic(type, &value)
+peek_type(&type, &contents)
}
class GHashTable {
+insert(key, value)
}
class service.c {
+sd_bus_read_dict(msg, **map)
}
sd_bus_message <.. service.c : uses
GHashTable <.. service.c : uses
Flow diagram for sd_bus_read_dict dictionary parsingflowchart TD
A["Start sd_bus_read_dict(msg, **map)"] --> B["Create new GHashTable"]
B --> C["Enter ARRAY container '{sv}'"]
C --> D{"Next DICT_ENTRY?"}
D -- Yes --> E["Read key (string)"]
E --> F["Peek variant type"]
F --> G["Enter VARIANT container"]
G --> H["Read value (string)"]
H --> I["Insert key/value into GHashTable"]
I --> J["Exit VARIANT container"]
J --> K["Exit DICT_ENTRY container"]
K --> D
D -- No --> L["Exit ARRAY container"]
L --> M["Return result"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Duplicate the key and value strings (e.g. with g_strdup) before inserting into the hash table or ensure you manage the message strings’ lifetimes to prevent dangling pointers.
- Add error‐path cleanup to free the hash table and exit any open containers on failure to avoid resource leaks.
- Validate the incoming variant type is SD_BUS_TYPE_STRING (or handle other types) before reading to avoid unexpected type errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Duplicate the key and value strings (e.g. with g_strdup) before inserting into the hash table or ensure you manage the message strings’ lifetimes to prevent dangling pointers.
- Add error‐path cleanup to free the hash table and exit any open containers on failure to avoid resource leaks.
- Validate the incoming variant type is SD_BUS_TYPE_STRING (or handle other types) before reading to avoid unexpected type errors.
## Individual Comments
### Comment 1
<location> `src/plugin-sdbus/plugin-ipwatchd/service.c:66-75` </location>
<code_context>
+int sd_bus_read_dict(sd_bus_message *msg, GHashTable **map) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential memory leak due to missing cleanup of GHashTable on error.
Ensure the hash table is freed if an error occurs before function completion to prevent memory leaks.
</issue_to_address>
### Comment 2
<location> `src/plugin-sdbus/plugin-ipwatchd/service.c:95` </location>
<code_context>
+ r = sd_bus_message_read_basic(msg, SD_BUS_TYPE_STRING, &value);
+ if (r < 0)
+ return r;
+ g_hash_table_insert(*map, (gpointer)key, (gpointer)value);
+ r = sd_bus_message_exit_container(msg);
+ if (r < 0)
</code_context>
<issue_to_address>
**issue (bug_risk):** Possible lifetime issue with key and value pointers.
Since the hash table stores pointers managed by sd_bus_message, freeing or reusing the message may invalidate them. Duplicate the strings before insertion to avoid dangling pointers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| r = sd_bus_message_read_basic(msg, SD_BUS_TYPE_STRING, &value); | ||
| if (r < 0) | ||
| return r; | ||
| g_hash_table_insert(*map, (gpointer)key, (gpointer)value); |
There was a problem hiding this comment.
issue (bug_risk): Possible lifetime issue with key and value pointers.
Since the hash table stores pointers managed by sd_bus_message, freeing or reusing the message may invalidate them. Duplicate the strings before insertion to avoid dangling pointers.
robertkill
previously approved these changes
Nov 12, 2025
Added missing sd_bus_read_dict implementation for D-Bus message parsing. This function reads a dictionary from a D-Bus message and stores it in a GHashTable. The implementation handles the D-Bus array of dictionary entries with variant values, properly navigating the message containers and extracting key-value pairs as strings. The function was missing from the codebase but needed for complete D-Bus functionality. It follows the standard sd-bus API patterns and includes proper error handling for all D-Bus operations. Influence: 1. Test with valid D-Bus messages containing dictionary data 2. Verify error handling with malformed or empty dictionaries 3. Test memory management and hash table creation 4. Verify proper container navigation and cleanup 5. Test with different dictionary sizes and key-value types feat: 实现sd_bus_read_dict函数 添加了缺失的sd_bus_read_dict实现，用于解析D-Bus消息。该函数从D-Bus消息中 读取字典并将其存储在GHashTable中。实现处理带有变体值的字典条目数组，正确 导航消息容器并提取字符串类型的键值对。 该函数在代码库中缺失但为完整的D-Bus功能所必需。它遵循标准sd-bus API模 式，并包含对所有D-Bus操作的适当错误处理。 Influence: 1. 测试包含字典数据的有效D-Bus消息 2. 验证对格式错误或空字典的错误处理 3. 测试内存管理和哈希表创建 4. 验证正确的容器导航和清理 5. 测试不同字典大小和键值类型
deepin pr auto review我来对这个diff进行仔细的审查：
代码质量方面的问题：
*map = g_hash_table_new(g_str_hash, g_str_equal);如果在后续操作中出现错误，这个hash_table不会被释放。应该在错误处理中添加清理代码。
建议改进后的代码： int sd_bus_read_dict(sd_bus_message *msg, GHashTable **map) {
int r = 0;
GHashTable *temp_map = NULL;
if (!msg || !map) {
return -EINVAL;
}
temp_map = g_hash_table_new(g_str_hash, g_str_equal);
if (!temp_map) {
return -ENOMEM;
}
r = sd_bus_message_enter_container(msg, SD_BUS_TYPE_ARRAY, "{sv}");
if (r < 0)
goto cleanup;
while ((r = sd_bus_message_enter_container(msg, SD_BUS_TYPE_DICT_ENTRY,
"sv")) > 0) {
const char *key;
const char *value;
const char *contents;
r = sd_bus_message_read_basic(msg, SD_BUS_TYPE_STRING, &key);
if (r < 0)
goto cleanup;
r = sd_bus_message_peek_type(msg, NULL, &contents);
if (r < 0)
goto cleanup;
r = sd_bus_message_enter_container(msg, SD_BUS_TYPE_VARIANT, contents);
if (r < 0)
goto cleanup;
r = sd_bus_message_read_basic(msg, SD_BUS_TYPE_STRING, &value);
if (r < 0)
goto cleanup;
g_hash_table_insert(temp_map, (gpointer)key, (gpointer)value);
r = sd_bus_message_exit_container(msg);
if (r < 0)
goto cleanup;
r = sd_bus_message_exit_container(msg);
if (r < 0)
goto cleanup;
}
r = sd_bus_message_exit_container(msg);
if (r >= 0)
*map = temp_map;
cleanup:
if (r < 0 && temp_map)
g_hash_table_destroy(temp_map);
return r;
}安全性方面：
g_hash_table_insert(temp_map, g_strdup(key), g_strdup(value));
性能方面：
其他建议：
这些改进将使代码更加健壮、安全和可维护。 |
yixinshark
approved these changes
Nov 12, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added missing sd_bus_read_dict implementation for D-Bus message parsing. This function reads a dictionary from a D-Bus message and stores it in a GHashTable. The implementation handles the D-Bus array of dictionary entries with variant values, properly navigating the message containers and extracting key-value pairs as strings.
The function was missing from the codebase but needed for complete D-Bus functionality. It follows the standard sd-bus API patterns and includes proper error handling for all D-Bus operations.
Influence:
feat: 实现sd_bus_read_dict函数
添加了缺失的sd_bus_read_dict实现，用于解析D-Bus消息。该函数从D-Bus消息中
读取字典并将其存储在GHashTable中。实现处理带有变体值的字典条目数组，正确
导航消息容器并提取字符串类型的键值对。
该函数在代码库中缺失但为完整的D-Bus功能所必需。它遵循标准sd-bus API模
式，并包含对所有D-Bus操作的适当错误处理。
Influence:
Summary by Sourcery
New Features: