Add library API #115
Add library API #115
Conversation
tasleson
force-pushed
the
library_rfc
branch
from
1d36bca
to
fe23113
Compare
|
@tasleson, now I looked into that and I see that you wrote: Should I revert SES support then? |
|
@mtkaczyk I'll fix this PR, no need to revert SES PR. |
tasleson
force-pushed
the
library_rfc
branch
from
fe23113
to
e5c52a0
Compare
|
Hi, Thanks, |
There was a problem hiding this comment.
HI @tasleson
I looked into it briefly. Please let me know what you think about my first findings.
Please also note that it is my first experience with library implementation. I'm learning by analyzing your code.
Thanks,
Mariusz
src/led/libled.h
Outdated
| * | ||
| * Note: Needs to be followed with led_flush() | ||
| */ | ||
| led_status_t LED_SYM_PUBLIC led_get(struct led_ctx *ctx, const char *path, enum led_ibpi_pattern *ibpi); |
There was a problem hiding this comment.
I think that we can drop this led_get and led_set in favor of led_get_slot and led_set_slot.
The result should be same. If you want to avoid changing it across all code- please mark those functions as backward compatibility only.
We should also export function for getting default controller (it is hidden now in led_get and led_set)
What do you think?
There was a problem hiding this comment.
Not all LED supported hardware supports get/set slot functionality, that's why this was included.
There was a problem hiding this comment.
led_get() - I can see that it reads current device->ibpi for device. That won't work in all cases. This is rather an interal ledmon value that the field we can rely on. At the begining it represents expected state for the device, the real get functionality was added by ctrl_get_slot. For me we should not export that.
led_set() - The subset of options for led_set is smaller than for slot_set, so simply, if we don't have set_slot we can fallback to old _write method. If slot property is not supported then we can throw error. For slots we can always pass device, it is supported. IMO library should not limit that.
|
@tasleson I would like to present you my point of view of the library implementation. Legend:
As you can see, there is no direct reference between CTRL and DISK but you can use DISK to retrieve slot identifier for a controller to blink appropriative led.
may be different but the concept is same. If you want to update led for device using particular controller you need to determine slot. You can send just block device (if any) and slot will be detected automatically for you. I also determined two problematic scenarios:
That is why for library I want to force user to determine controller for device. We can provide something like
For me, that is all what library should provide. We should allow user to just set LED for proper slot. The library implementation goals I determined:
I understand that the concept I provided will require more changes. We should separate library content and ledmon/ledctl content more. Let me know what do you think. |
In my opinion we need to make the library as simple as possible for someone to use. If there is complexity, it should be in the library implementation, not pushed to the application developer. When I first approached the project I mentioned a use case where you supply a device node and a pattern, discussed here: #97 (comment) . Without that functionality I cannot simply drop it into libStorageMgmt as a replacement. This is a use case which any developer should be able to utilize very quickly. In your latest discussion you outline removing the functionality of the get/set by device node. Is it your intention that we are then removing support for all the others? So only NPEM, SES, VMD would be supported? The notion of a controller and slot is useful for changing the state of LEDs when the block device no longer has a device node. The API should be simple to understand and utilize for this use case too. You mention:
The function You also mention:
I believe I actually removed this from ledctl. I'm thinking that maybe I've introduced a bug in doing so. Maybe the API isn't the best fit for ledctl/ledmon, but I think we should be striving for something that doesn't require the API user to (If I'm understanding your diagram correctly)
Ultimately what I was trying to achieve is something simple, with enough functionality that people would find it useful and have broad hardware support. I also wanted to minimize code churn in the code base, to hopefully reduce the introduction of bugs. |
|
Re-based and moved the commit for removing exclusionary language to beginning of changes. When #118 gets integrated I'll remove that change from this PR. I've not addressed any of other other comments at this time. |
Yeah, the initial discussion took place before we developed "get/set-slots" and I deeply realized how it all is connected. The current approach I proposed is based on my current experience. But no offence, we need all to be on the same page to make library implementation possible. I don't want to repeat myself and discuss things many times because my current view is quite different. I did the schema to visualize my mind to myself too.
Yup, in my approach I proposed to use opaqued Beside the my personal opinion about the controllers I can see that all current code bases on default one. It seems that I want to create dead api. So, maybe instead giving possibility to choose controller now, we can create set of functions prefixed by
No, but they have
Similar to above, it is called by
Agree now, as above- we shuld just put
Sure and I will support you with that but I also know if I don't handle this and don't force some good requirements from the start then we will end with a huge and understandable library with set of not needed functionalities inside. I know this code well so I'm trying to describe you what is for applications and what is for library. No worries about bugs, we don't have unit test they are unavoidable. We need to deal with that. |
I'm trying to understand better why we need to expose the idea of default controller. From your perspective, it seems like we must expose it and not handle it internally. Is VMD & NPEM the only one that has the requirement of using NPEM? In this case NPEM is the default yes? Are you wanting the ability for the library user to have the choice to use VMD or NPEM to change LED states instead of allowing the library to select the best one for a given hardware configuration? In SES with multi-path, there are multiple controllers and paths to change the LED state for a specified slot in a given enclosure, but I wouldn't say one is better than another, unless one of the paths is down. I'm trying to wrap my head around why ledctl works with all supported hardware when you do something like: # ledctl normal=/dev/sdrand the user doesn't need to worry about a controller. I need more of an explanation to why we can't have the library do that too? Thanks |
Initially I wanted to give possibility to choose the controller explicitly as you said. But during discussion I lowered this requirement because it is not needed now, let me link all my comment:
Here explanation: We have allowed/ denied controllers list- that gives some flexibility, we are unable to decide how to blink for particular disk but we can exclude the controller- should be sufficient in standard cases. When someone will come with this problem that will require to explicitly set the controller we will add new methods to library API. |
Agreed. The API can always be added/extended, I fully support the idea of not adding something until it's needed. |
|
This is not a complete representation, I focused here on ledmon mostly but I also wanted to list all expected library functions. This is why some of them doesn't have references. Thanks! |
|
@mtkaczyk Please take a look at updated PR and see if this is starting to look closer to something you like. |
There was a problem hiding this comment.
@tasleson
Yes, it is going well. Thanks!
I've put some new comments please take a look.
The one objection I have is around accessing structures by ledmon and ledctl defined in libled_internal.h. I'm fine with including it in both applications for backward compatibility but I think that we should avoid accessing structures defined there as much as possible.
For example:
while(LED_STATUS_SUCCESS == (status = led_slot_list_next(ctx, &slot))) {
if (slot_req->cntrl == slot->cntrl)Perhaps, we should consider new method in libled.h slot_get_controller to avoid referencing slot content directly? I've put comment there too.
There was a problem hiding this comment.
HI @tasleson,
I tried to review it deeply. Please see a comments.
The next step (after addressing current issues) should be removing ledmon and ledctl lists from library ctx (slaves, volumes ledmon/ledctl block_list).
As the last step, I would like to propose code dividing into subdirectories:
- common (the files used by applications and library (utils, lists)
- ledmon (ledmon files, ledmon.c, udev.c, udev.h)
- ledctl (ledctl files)
- lib (library content)
Let me know if you have any objection or advises.
As I said- it is my first experience with library.
Thanks,
Mariusz
This may very well be a good change, but from the perspective of the public API, it's an implementation detail. As such, I would like to defer it until we get this PR merged.
I'm not an autoconf wizzard, I don't muck with it enough to be super proficient. I'll spend some time on it, but if it takes to long I would like to defer and place it a separate PR after this gets merged.
|
tasleson
force-pushed
the
library_rfc
branch
3 times, most recently
from
d595980
to
421f496
Compare
|
@mtkaczyk I build a local SRPM and ran it through our code analysis tools. It found ~34 defects which I'm attempting to work through. Some look like false positives, but some look like genuine bugs (memory leaks, possibility of de-referencing null, failure to check return codes etc.) I'll try to get that completed tomorrow and do testing as you suggested on the command line. |
There was a problem hiding this comment.
This may very well be a good change, but from the perspective of the public API, it's an implementation detail. As such, I would like to defer it until we get this PR merged.
Got it. Looks reasonable.
@mtkaczyk I build a local SRPM and ran it through our code analysis tools. It found ~34 defects which I'm attempting to work through. Some look like false positives, but some look like genuine bugs (memory leaks, possibility of de-referencing null, failure to check return codes etc.)
Great! My infrastructure is going back so I will run my own analysis later. Generally, it is a finishing touch.
I'm not an autoconf wizzard, I don't muck with it enough to be super proficient. I'll spend some time on it, but if it takes to long I would like to defer and place it a separate PR after this gets merged.
OK, agree. We can handle it in separate pull eventually.
Also list.c is used by library code and ledmon/ledctl, so it should be in the lib dir too. I've added a commit to the end which does the re-org, but I'll likely remove it with future changes or simply squash the entire PR to one commit to address any needed changes.
Ok.
I need to do internal testing of slots management feature, then I will release new ledmon version. After that I will take you PR to baseline. I need around one month.
Led me know if that works for you.
src/libled.c
Outdated
| led_status_t led_slot_list_next(struct led_ctx *ctx, struct led_slot_list_entry **slot_entry) | ||
| { | ||
| if (!ctx->sl.iter) { | ||
| led_status_t rc = led_slots_get(ctx); |
There was a problem hiding this comment.
I proposed to make it configurable via parameter passed to scan:
led_status_t LED_SYM_PUBLIC led_scan(struct led_ctx *ctx, bool scan_slots);For reset and next we will return error if ctx->sl.slot_list.head is NULL.
My goal is to left all scans in led_scan() because I want highlight that it is the one place when we are iterating over the system (sysfs, ioctls and others).
The later we should impose led states only, assuming that we have all necessary information to do it.
Let we know how you see it.
src/block.c
Outdated
| if (!device) | ||
| goto error; | ||
|
|
||
| struct _host_type *hosts = cntrl ? cntrl->hosts : NULL; |
There was a problem hiding this comment.
cntrl cannot be NULL here:
if (!cntrl)
goto error;There was a problem hiding this comment.
And we are following kernel coding style, definitions should be placed at the beginning of function
There was a problem hiding this comment.
I proposed to make it configurable via parameter passed to scan:
led_status_t LED_SYM_PUBLIC led_scan(struct led_ctx *ctx, bool scan_slots);For reset and next we will return error if ctx->sl.slot_list.head is NULL.
My goal is to left all scans in led_scan() because I want highlight that it is the one place when we are iterating over the system (sysfs, ioctls and others).
The later we should impose led states only, assuming that we have all necessary information to do it.
Let we know how you see it.
I actually prefer what I did the first time. Where the API caller called into the API and got an abstraction of a list and then iterated on it. This was done as we don't want to expose a linked list implementation.
Regardless, there is no sysfs scan performed in either the implementations I've provided. It simply walks over the in memory representation of what the scan already did and presents it with opaque data types.
Before I re-write this for the third time, please consider what the API end user experiences, how it can be used, the limitations of it etc. For example, in the first implementation a user could scan, get the list, close the library handle and continue to use the list abstraction indefinitely. The API user could also have more than one at a time too, representing change over time. Thus for example, the functionality where ledmon keeps track of state and, adds/updates/removes could be assisted by what the API provides.
There was a problem hiding this comment.
Before I re-write this for the third time, please consider what the API end user experiences, how it can be used, the limitations of it etc. For example, in the first implementation a user could scan, get the list, close the library handle and continue to use the list abstraction indefinitely. The API user could also have more than one at a time too, representing change over time. Thus for example, the functionality where ledmon keeps track of state and, adds/updates/removes could be assisted by what the API provides.
I believe that context is always must have. The main reason is:
- Always use a library context object. Every thread should then
operate on its own context. (https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git/plain/README)
That is the guidance I would like to follow to limit complexity for now but maybe I've taken in too serious. I understand that the approach you proposed is more flexible and could save some memory. To achieve that, we can handle two contexts and compare them. Nowadays we don't need to save every possible memory cell :)
The main purpose of the library is to allow led patterns manipulation. To achieve that context is always needed.
I see your first approach could be valuable for ledmon where we need to compare two system scans. I think that we can handle that in separate pull If we will determine that there is real use case for that because the first question we should ask ourselves is: Do we have user for the new API?
First, I would like to focus on removing ledmon code from lib and we agreed to handle that later. Then, we will know which list can be eventually coped for the user and if it is really needed (we have request or need to do it).
In this comment I wanted to discuss moving slots list generation to led_scan(), the discuss goes into other direction.
Let me know how you see this problem.
Thanks for giving you personal PoV- I really appreciate that.
There was a problem hiding this comment.
The code was walking the systems/slots and allocates an internal structure for each item in the list. It only contains the information presented in the public API, so it's much smaller than what is present internally. Its not a clone of anything. One of the main reasons this is done is that would be very difficult to handle the case where a user gets the iterator, and during iteration they do a
led_scan. That could potentially invalidate one or more internal structures if we simply provide iterating over the internal data structures if devices are added/removed.
You right, it is not exact clone, it is a new list made from original one. "Clone" was not perfect wording for that.
IMHO we should accept the risk you mentioned. I think that it is not a problem. User should know that after led_scan() pointers will be invalidated and if user need old list then he must make a list copy itself. The solution for the problem should stay in application, not in library.
The user should decide whose properties are needed, and should be copied to the internal temporary list. All we need is to provide API to access them. It also gives a direct control of allocated memory (application allocates, application frees).
I'm not sure what the problem is with making an iterator public. This is simply a place holder for their existing location in the internal list. The main point of exposing iteration is we then don't expose any kind of linked list structure(s). Iterators can be used across different languages bindings too.
Sorry wrong word. I meant "something what need to be resolved/ addressed", maybe concepts / ideas were better. Sorry for confusing you.
I'm confused by this. The public API as access to iterator, eg.
next. We can easily add apreviousandresetto what I initially did. If the user wanted to do nested for loops they would need to get two different list instances. At the moment I'm struggling to see the use case, but they could do it if they needed to.And as you said- it is more intuitive. I have strong feeling that I don't remember correctly your first implementation and that is all based on my incorrect assumption that you returned copy of list to the caller. I cannot verify that after squashing. I'm sure that I saw it somewhere but I don't remember exactly where.
I still have an old branch. I can resurrect and implement
previousandreset. I'll also include a client example showing its use. I have one already that I'm using for testing.
You don't need to implement functionalities that are not needed. The goal of my comment was to find examples where public iterator brings are real advantage, even if they are not needed now.
Conclusion: I agree that your first approach were better but I would like to not use "internal structure for each item" just present everything as is and document that pointers will be invalidated after led_scan().
There was a problem hiding this comment.
Conclusion: I agree that your first approach were better but I would like to not use "internal structure for each item" just present everything as is and document that pointers will be invalidated after led_scan().
I've modified the PR. The controller list abstraction is using the internal data structures without making a copy. This was pretty straight forward and the code is actually smaller. However, the slot data is not so easy.
- Some of the information is generated, it doesn't exist in any current data structures like the slot id and the current LED status which needs to be generated/retrieved. Thus if we don't take the current approach of allocating a list of structures which store this information when we iterate through the code to retrieve it, we are then forced to require the API user to check a status code during list iteration to see if any errors occurred as the data would need to be retrieved during the
nextorpreviousiteration. From an API user perspective, it's much nicer to retrieve the list and check the status once that all is good and then simply walk the data without having to constantly check if it's ok. - In my first implementation the API user would specify the controller to retrieve slots for. Later I changed this to simply retrieve all slots on the system as the returned slot information includes the enumerated controller type. From an API user perspective I believe this is better. Otherwise the API user needs to loop across the different supported controllers that support the slots API and retrieve them individually. To do this iteratively without allocating a new list structure will require some really interesting code as we will need to internally iterate over three different linked lists, each of which contains dissimilar data.
I'll continue to investigate ideas on how to implement this without it being a significant increase in complexity, but I'm not seeing it at the moment.
Update: I'm in the process of implementing this for slots. We will however need to have a return code for each iteration. I'm also thinking we should change the controller iterator to match even though it doesn't need a return code, to make them consistent.
There was a problem hiding this comment.
Do not fear to make a slots list inside the context to contain all necessary properties -that fits the design I want.
I don't want to return copied list, with no reference inside the context. If you need to create additional list inside context to combine all necessary properties for slots- that is fine.
There was a problem hiding this comment.
Do not fear to make a slots list inside the context to contain all necessary properties -that fits the design I want. I don't want to return copied list, with no reference inside the context. If you need to create additional list inside context to combine all necessary properties for slots- that is fine.
I'm not exactly sure what you're saying here. Regardless, I'm going to leave this PR the way it is as I don't think I can reduce the list elements for listing slots. I've completed a different implementation that generates the slot data during iteration and placed it in this branch: https://github.com/tasleson/ledmon/tree/library_rfc_less_alloc . This branch isn't complete, as I would want to change the signature of the controller iteration to report back status during each iteration too. This is implemented with a linked list of linked lists with forward/backwards iteration included.
Please take a look at both and let me know which direction you prefer.
There was a problem hiding this comment.
I'm not exactly sure what you're saying here.
Apologize for being inaccurate. Probably the root cause it that I'm only the reviewer. I will analyze what you prepared and eventually I will come with my code, to not bother you more with more comments. Then we will choose the way.
Please take a look at both and let me know which direction you prefer.
I need few days to do that.
|
@mtkaczyk The directory re-work is in the PR |
That works, thank you for giving a time estimate. |
tasleson
force-pushed
the
library_rfc
branch
2 times, most recently
from
3150ba0
to
877ec20
Compare
|
Example of using library API #include <stdlib.h>
#include <stdio.h>
#include <led/libled.h>
static inline void print_slot(struct led_slot_list_entry *s)
{
printf("cntrl type: %d: slot: %-15s led state: %-15d device: %-15s\n",
led_slot_cntrl(s), led_slot_id(s), led_slot_state(s), led_slot_device(s));
}
static inline void print_cntrl(struct led_cntrl_list_entry *s)
{
printf("cntrl type: %d: path: %s\n", led_cntrl_type(s), led_cntrl_path(s));
}
int main() {
struct led_slot_list_entry *slot = NULL;
struct led_ctx *ctx = NULL;
struct led_slot_list *slot_list = NULL;
struct led_cntrl_list *cntrl_list = NULL;
struct led_cntrl_list_entry *cntrl = NULL;
led_status_t status;
status = led_new(&ctx);
if (LED_STATUS_SUCCESS != status){
printf("Failed to initialize library\n");
return 1;
}
status = led_scan(ctx);
if (LED_STATUS_SUCCESS != status){
printf("led_scan failed %d\n", status);
return 1;
}
status = led_slots_get(ctx, &slot_list);
if (LED_STATUS_SUCCESS != status) {
printf("led_slots_get failed %d\n", status);
return 1;
}
printf("Printing slots in order returned\n");
while ((slot = led_slot_next(slot_list))) {
print_slot(slot);
}
led_slot_list_reset(slot_list);
printf("Printing slots in reverse order\n");
while ((slot = led_slot_prev(slot_list))) {
print_slot(slot);
}
led_slot_list_free(slot_list);
printf("Printing controllers\n");
status = led_cntrls_get(ctx, &cntrl_list);
if (LED_STATUS_SUCCESS != status) {
printf("led_cntrls_get failed %d\n", status);
return 1;
}
printf("Printing controllers in order returned\n");
while((cntrl = led_cntrl_next(cntrl_list))) {
print_cntrl(cntrl);
}
led_cntrl_list_reset(cntrl_list);
printf("Printing controllers in reverse order\n");
while((cntrl = led_cntrl_prev(cntrl_list))) {
print_cntrl(cntrl);
}
led_cntrl_list_free(cntrl_list);
led_free(ctx);
return 0;
} |
Show the configure option and show a brief example on compiling/ building/running it by adding a new markdown file in the lib directory. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Using the C testing library check, add some unit tests to test the functionality of the library. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Library users will expect that when they change a value of an LED that if retrieve the value after the set that is will reflect its new value. Additionally a library user should be able to seamlessly use the slot or device path API if the LED hardware supports it and have the ibpi values match. This change allows SES hardware to meet these expectations. Signed-off-by: Tony Asleson <tasleson@redhat.com>
This allows us to run both the unit test and the command test with `make check` Signed-off-by: Tony Asleson <tasleson@redhat.com>
Function led_free was being called in _ledmon_fini() and main. Remove duplicate call in main. Signed-off-by: Tony Asleson <tasleson@redhat.com>
When the device is empty we get the following exception Exception: Text line 'slot: sg3-0 led state: NORMAL device:' did not match regex '^slot: (.+)led state:(.+)device:(.+)$' Signed-off-by: Tony Asleson <tasleson@redhat.com>
The library check is needed for the library unit test which was added. Signed-off-by: Tony Asleson <tasleson@redhat.com>
When compiling -g -O0 we get a linker error: /usr/bin/ld: ledctl-ledctl.o: in function `_ibpi_state_init': ledmon/src/ledctl/ledctl.c:318: undefined reference to `list_append' Corrected with making this inline function static which is described: http://gcc.gnu.org/onlinedocs/gcc/Inline.html Signed-off-by: Tony Asleson <tasleson@redhat.com>
Issues found during valgrind testing the unit test on SES hardware. Signed-off-by: Tony Asleson <tasleson@redhat.com>
There was a problem hiding this comment.
Hi @tasleson
We finished the tests, I needed some time to analyze the list of defects to exclude false defects, and those requiring changes in test cases and unrelated to your changes. I have good news, we did not find any issues related to the main functionalities.
In this PR, there were problems with logging, which I described in the appropriate places.
during the analysis, I noticed slight differences in the style of function descriptions, I added comments where something needs to be tightened.
I haven't analyzed your last discussion with Mariusz, so give me a moment to familiarize and see if I have anything to add.
Thanks.
Log severity was incorrectly set. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Ensure we have handled retrieving all the options before we load the library preferences. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Instead of returning a pointer to memory that contains no string we will return NULL when this field is empty. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Missed setting the library log level from the configuration. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Make this clearer on why we are adding an additional byte to the allocation. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Make this conform to established code standard. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Change from bool to enum led_cntrl_type for led_is_management_supported which allows us to return not only if it is supported, but what controller provides the preferred support for it. Remove led_set as this doesn't work for all the different supported hardware. You need slot support to have the ability to retrieve the LED status. Signed-off-by: Tony Asleson <tasleson@redhat.com>
Remove the calls to get_led as we removed that from the API. Signed-off-by: Tony Asleson <tasleson@redhat.com>
|
This moment finally comes. It has enough good quality to be merged. Thanks to all contributors (especially @tasleson for driving this great feature). It is small step for me to click merge but is is a big step for ledmon project! I totally understand that it is not yet complete, there will be a bugs. There are still some topis that require clarification. There are still many questions. We will drive them all in separate pulls according to our needs and priorities. I'm not going to make a release soon. There is a time for corrections in the library API. Thanks everyone for support! Tony, please give me an ack, then I will merge it. |
My official |
This is an initial pass at providing a library interface. This PR is based on ses_slot_support #109 , that PR should be merged first. The number of changes is fairly significant and I'm assuming it will take a bit to get this fully reviewed, tested, and integrated. The first few commits (starting with f555300) are fairly invasive as it wasn't easy to constrain the changes when introducing a library context and support logging and get things to compile. The early commits disable compiling of
ledmonin the makefile until the library was more complete, before re-enabling and updating. I tried to break out the commits to finer granularity a couple of times and gave up. As things progressed, I made smaller more contained edits to address specific issues.Ledctlhas good separation now and uses the public library interface.Ledmonis still tightly coupled with the code base. Eventually it maybe be useful to add the needed functionality to the public library interface to supportledmon, but I don't think it's required.I've only done limited testing with
ledctlandledmonon a system I have with SES. I'll need to check around to see if I happen to have access to other hardware which is supported for further testing. I also wrote a simple program which uses the library to ensure some basic functionality. As we expand testing and examples, I think it would be good to include them in the source repository.I followed the guidelines in libabc when crafting the API and previous discussions we had on the issue ref. #97.
Resolves: #97