[NOT PLANNED]adapt nginx attach example with new agent_config #384
Conversation
There was a problem hiding this comment.
I prefer to fix the SIGSEGV issue rather than downgrading the implementation, I'll fix it
I think sometimes keep it simple and stupid would be better? If the agent needs shm to init and become complex, and requires user to Initialize it with special approachs to avoid error, it will
- make software maintainance harder
- make library user confuse
I still think maybe using raw char buffer would be better, easiler to understand and maintain.
What if the buffer overflows?
|
We can add wrapper functions to help people setup it.
Do you think a library user may need to setup the agent_config mannually? If yes, put things in shm may increase their difficulty. |
|
(I will rebase this to the latest one, please do not close it) |
We can change |
If the setup config has clear API interface and user don't need to directly deal with the shm API, use shm string approach would be better. Don't make it has two options, you can do always store things on shared memory. Do you think we can make the init API more clear and easy to use? it seems like user need to do many things carefully in their library, and we don't have documents to explain them, like: Can we simpilfy some API and provide some comments? |
|
云微 ***@***.***>于2025年4月30日 周三上午8:39写道：
*yunwei37* left a comment (eunomia-bpf/bpftime#384)
<#384 (comment)>
We can change agent_config default to store things on shared memory, with
no arguments on constructor.
If the setup config has clear API interface and user don't need to
directly deal with the shm API, use shm string approach would be better.
Don't make it has two options, you can do always store things on shared
memory.
Do you think we can make the init API more clear and easy to use? it seems
like user need to do many things carefully in their library, and we don't
have documents to explain them, like:
https://github.com/eunomia-bpf/bpftime/blob/1c89400981b1fd41497e562b31f2f9e05bea2cc0/example/attach_implementation/nginx_plugin_adaptor/nginx_plugin_adaptor.cpp#L1C1-L148C2
Can we simpilfy some API and provide some comments?
—
Reply to this email directly, view it on GitHub
<#384 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACEWIGUZ344EV2KMRA3DH3L24ALVHAVCNFSM6AAAAAB3YMRJN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBQGUZTMNBZHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
Attach related parts has document in comments and eunomia.dev; I'll update
agent_config to make it store strings on shared memory by default
|
|
Documents about the attach part are described at
https://eunomia.dev/bpftime/documents/attach/
enustaH ukiM ***@***.***>于2025年4月30日 周三下午1:32写道：
…
云微 ***@***.***>于2025年4月30日 周三上午8:39写道：
> *yunwei37* left a comment (eunomia-bpf/bpftime#384)
> <#384 (comment)>
>
> We can change agent_config default to store things on shared memory, with
> no arguments on constructor.
>
> If the setup config has clear API interface and user don't need to
> directly deal with the shm API, use shm string approach would be better.
> Don't make it has two options, you can do always store things on shared
> memory.
>
> Do you think we can make the init API more clear and easy to use? it
> seems like user need to do many things carefully in their library, and we
> don't have documents to explain them, like:
>
>
> https://github.com/eunomia-bpf/bpftime/blob/1c89400981b1fd41497e562b31f2f9e05bea2cc0/example/attach_implementation/nginx_plugin_adaptor/nginx_plugin_adaptor.cpp#L1C1-L148C2
>
> Can we simpilfy some API and provide some comments?
>
> —
> Reply to this email directly, view it on GitHub
> <#384 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACEWIGUZ344EV2KMRA3DH3L24ALVHAVCNFSM6AAAAAB3YMRJN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBQGUZTMNBZHE>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
Attach related parts has document in comments and eunomia.dev; I'll
update agent_config to make it store strings on shared memory by default
|
I can create some wrapper API to make creating attach implementation more easier, but to archive maximum custimization, the original API should be kept |
After a thorough check, I got the following conclusion:
What do you think? @yunwei37 |
If that's true, I think it would be better if we can just use char[] buffer with functions to avoid overflow. |
C-arrays are not able to expand, so if a user want to store more things (such as a very long log output path), the user will fail, can this be accepted? |
Also, do you have any suggestions about |
Typically Linux has a maximum path length of 4096 characters. So we don't need to worry about that. |
I think that's a good idea! |
Officeyutong
changed the title
yunwei37
force-pushed
the
master
branch
2 times, most recently
from
cbcb1e9 to
20acd88
Compare
2 participants
In the new
agent_configdesign introduced in #371 , construct anagent_configwithout any argument will make it store strings on a local heap. If a user plan to store strings on shared memory,agent_configshould be initialized withboost::interprocess::managed_shared_memory &. The nginx attach example didn't adapt this, this PR fixes it