-
Notifications
You must be signed in to change notification settings - Fork 123
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
comments on highlevel API and LCDproc work #2772
Comments
Thank you very much for your review!
I think it is a good default to have names which are safe from collisions. Do you want for LCDproc LCDEXEC_ADDRESS instead?
Yes, I love them because we want to provide APIs with smooth upgrade experience. In the concrete case of getters we could make an exception (we already do this in the C++ code-generation). But I see this as an optimization which only should be done if we see that it is relevant for LCDproc. (@kodebach will benchmark this.) From my experience it is only relevant if the getters are called very often in tight loops. If the getters are only used a few time in the startup procedure it should not matter.
That types like "int" have different sizes on different platforms but we need to know what we allow in the config file.
Yes, we had long discussions about this. @domhof was very much convinced that it is much better for the usability of the API. If you do not pass the error object, the API call will exit, which makes it very hard to use it wrongly. I am not sure if this is fully implemented now, though. (@kodebach?)
It is also that we call other mallocs when errors happen. The concept of the API would be fail safe: If the malloc for the error object fails, a null pointer will passed to the API and then Elektra will exit on a failure. But this is untested, though. Is it important that this works?
Good point, the API docu must be very clear about that, I created #2774. |
Sure, but as long as the compiler detects them, nothing really bad can happen.
LCDEXEC_ADDRESS or CONF_ADDRESS both seem reasonable to me.
This was mostly a general remark. I don't thing lcdproc is affected any worse then others.
My experience is quite the opposite: In a tight loop, when everything is in cache, a few extra instructions hardly matter - even on the ARM CPUs I'm mostly using. It's loading code into the cache in the first place, that is the actually expensive operation. Of course for initialization code, that's not an issue either. In this context I'm only concerned about the size of the resulting binaries: Compiler optimization can do many things, but it can't work around a API. Using opaque data structures costs about 20 bytes per member access over the transparent case. A RAM cost that has to be paid even if the access is in some rarely used error path and a flash cost that has to be paid even if the code isn't running at all. Multiply this with the number of accesses in a typical application and the number of systems where the application is running, and you easily get a rather big number. There are certainly cases where spending such resources for extra flexibility is reasonable. But in many other cases it's not and only happens out of habit.
Yes, "int" is bad for stored data. But AFAIK the other types like "long" don't have the same problem. And what's more important, there are already the types from "stdint.h" with explicit size, that are widely known. I'd recommend you use those. Inventing your own just makes the code harder to read for everybody.
Of course not, I still advocate to treat malloc() of small data structures fail safe after all. I wouldn't have commented on this, if it wasn't for the discussion last year. |
Yes, now you get quite good error messages even with macros. Some years ago the compiler error did not indicate how macros manipulated something: Then it sometimes needed a lot of time to find out that macros are involved.
There is a huge difference if the API is used for every config access compared to if an application copies the config data into structs. Affected will be mostly applications that do not copy. And not copying is actually recommended if you want on-the-fly updates of config.
Exactly, so I wonder why your experience is contrary? If our getter would return some member of a struct and can be inlined, there would be no API cost. If you only pay the API cost in a constant number of cases it is very different to applications where an unbounded number of API calls can be made (which do not copy).
For C99 or if stdint.h is available, we typedef to them. So we could use these fixed-sized-types if you only support systems that have C99 or stdint.h. |
I hope, I addressed everything... Things that can actually be changed(without redesigning large parts of the API)
I agree, the current tags are very long. Sadly since C doesn't support any kind of namespaces, it is hard to avoid long names and at the same time avoid name collisions in general. I will add some options to manipulate the generated tags. Currently they are created using the prefix You also don't have to use the tags at all. If you take a look at how the tags are resolved you can will find multiple alternatives that resolve to the same inline function. Some of these alternative might be shorter. It should also be fairly safe to directly use the inline functions (e.g.
We could probably use the equivalent
This is actually quite complicated. See my answer in #2774. Concerning the API design
I am also not a fan of how Elektra uses structs. However, I did not have any influence on this decision. I did try to minimize the performance (and binary size) impact of the code-generation API. That is why all of the generated accessor functions are
What exactly do you mean? Please clarify what exactly you mean by "making an exception".
I absolutely agree. AFAIK the problem we are trying to solve is that C doesn't define fixed sizes for it's data types. The C standard only defines lower limits. Why such verbose names based on the CORBA types are used, I don't know. This decision was before my time and I have absolutely no idea, how this conclusion was reached. I get that by using our own types we retain some C89 compatibility (although I am fairly certain we are not strictly speaking C89 compatible, the code-generated API certainly isn't), but that is not a good enough argument against using However: As stated above, if C99+ is used all of the
The code-generation API more or less requires C99. We use things like
Error handling was also decided before my time. I wouldn't have done it this way either. However, I am definitely of the opinion that in most cases we can treat There is also currently a major refactoring of the low-level error handling under way, so some of the high-level error handling is going to change. This is also why currently the only publicly accessible property of errors is the description. Misconceptions
You don't have to pass an error object. You only pass an
Yes, not copying is far more wasteful. (see below)
On-the-fly updates (as in the notification API) are not supported in any way by the highlevel-API. Currently we only update the internal KeySet in I actually recommend calling Optimization ProblemsSadly my experience is that Elektra is very badly optimized and that if you really care about performance, you shouldn't use it (yet). IMO Elektra uses RAM usage is also not a strong suit of Elektra. For example each (°) This probably due to the fact that many people have worked on Elektra. Most of which probably weren't too concerned with performance. (°°) Having a hidden KeySet inside every Key is another thing I don't like. Most of the KeySet and Key functionality is not necessary to store metadata. (°°°) I guess we have to have |
Looks like github ate my reply via e-mail yesterday. Pasting it here again: Thanks for your instructions. I actually compiled elektra and lcdproc After compiling I compared binary sizes with and without your modifications
In some ways a disappointing result. Especially considering that the The main cause seems to be the code inside elektragen.c, but most other I also tried to take some timings, but any client segfaults immediatly. I haven't looked into it yet, but I guess the segfault will be fairly Here is what gdb says about the segfault:
|
It ended up over here #2748 (comment).
A part of that is the specification that is compiled into the binary. However there are still some redundancies. I will look into it. But if you want to keep binary size small, you will always be better of with your own specialised solution. Actually, it is quite pointless to compare the binary sizes alone, since you need to have the Elektra libraries installed as well. Those alone are bigger than the old LCDproc clients. You just have to make a decision here: Do you want all the features of Elektra or do you want small binaries? |
Actually, we only need a user-chosen prefix, and in LCDproc we would choose to not have a prefix.
We could make an exception that elektraGet does not make an API call.
It is documented in #1324.
This is because of how the implementation works, not because of the type system. The C++ generated code already shows that it could be different. But I am afraid this would be a reimplementation of elektraGet and some parts of code generation. The optimization is not so trivial (especially in C, where you would need to generate structs).
Maybe we should reconsider compiling-in the spec if the binary size is so important. (And simply fail if there is no spec)? Or have a compile switch to compile it in? @haraldg What do you think?
Most features of Elektra should not affect the binary size of the applications at all. The compiled-in spec, which provides the feature that applications also start without being installed, is an exception and not the rule. |
The type system (as in which types are available) is irrelevant yes. But "the way Elektras type system works" is not. What I mean is that the core of Elektra doesn't have a type system. Everything is a string. Why have type checking via the
We would just need to store the converted value inside the
I will add an option to the code-generation to disable it. But at some point we have to define what our goal is. Elektra simply cannot fulfill all use cases. Binary size, RAM usage and performance are all metrics where a specialized solution limited to the exact needs of an application will always outperform a general solution like Elektra. Another thing that might increase binary size is the fact that we generate setters for everything as well. I will add an option to disable setters, in case your compiler doesn't strip out these unused functions. |
Yes, this is how your implementation works. But you could already transform earlier and only return the value in elektraGet.
If you need a ksLookup you are too slow anyway. If you want it really fast, you need to generate a struct, fill it up in elektraOpen, and in elektraGet is a static inline function which returns the member of the struct. (Then you trivially also have all the guarantees.)
@kodebach Better wait for the answer of Harald. And first we need to find out what the causes of the big binary is, maybe it is not only the spec. I actually like your idea of compiled-in spec with specload very much. Thus I was also thinking that we could compress the spec but this would be way too much for your thesis now.
@kodebach Were you able to reproduce the large binary size? Please always first try to find the causes before you optimize. |
Hi, since I'm late to the party and you already discussed a lot, I won't respond to everything again. If you feel you miss a reply on something from me, please raise the issue again. Where does the size increase come from:Yes, strings are a big part. However the text section is much bigger then the ro-data section. I guess pushing all the arguments to the many calls of keyNew() onto the stack actually takes more space then the arguments themselvs. I just used the default CFLAGS of lcdproc, not sure which optimization they choose - maybe none. However I don't think that setters or other unsused code are an issue: The sizes of most objects only grow moderately - probably just due to extra function arguments and other minor changes. I manually compiled elektragen.c with On metrics and optimizationWe don't actually optimize lcdproc for anything, but try to not pointlessly use resources. There definitively isn't a strong size limit. However I do care about supporting embedded systems, after all I'm the OpenWRT package maintainer of elektra. It's true that the size increase is still small compared to the size of elektra itself. It probably would be insane to use elektra in lcdproc, if it is the only user on the typical system where lcdproc is installed. I'm gambling on elektra eventually taking off and being installed widely anyway, so the size of elektra it self doesn't count directly. However, if the trend of installed size of applications almost doubling continues and elektra does take off, I will need a new laptop just because of that. I don't like it. Also I feel like the size of elektra is justified, because it provides many nice features. But a huge size increase in application binaries is less justified: Right now we need twice the size in lcdproc to do pretty much what we did before. (Yes, we don't care much about the specification part of elektra.) The metrics I use are: installed size, binary size, memory footprint, (cache lines periodically filled), (instructions executed.) The last two are not relevant here, because elektra related code won't be execuated repeatedly. Of the other three binary size is kind of the least important: Not regularly used mmaped binary sections will just be dropped from RAM and not use resources permanently. binary size is mostly useful because it is very easy to measure and compare. installed size obviously takes a space on the flash chip and any memory that is not mmaped from the file system (ie malloc()ed data structures) has to be kept in RAM (unless you have swap space, which most systems don't) forever. Personally, I mostly spend optimization effors on cache related issues. But aside from general API considerations, that's not relevant for elektra. Possible improvements and workaroundsI'm lacking much of the context of design decisions behind elektra, so I probably can't contribute much to this part of the discussion. A few ideas, that float in my mind: Some time ago Markus and I discussed the possibility to use code generation with some kind of embedded mode: On embedded systems typically nobody is doing any configuration changes at all. All the (help) strings targetted for users and many of the checks aren't useful. Maybe this goes into the same direction as the "compile switch" suggested above? Maybe the specification can be stored in a more efficient format then construct it programmatically. (I hear you already complain about ABI stability issues, but it is worth considering at least.) Actually, this whole business of storing the specification in the binary (which binary even in case of modular applications?) and then printing it on stdout on demand seems rather roundabout to me. You could store it in an elf library and dynamically link it (and dl_open() it from kdb) or even store it in some other suitable and efficient format and load it in the elf constructor. (Assuming there actually is a reason not to just load it during elektraOpen(), which probably is what I'd have done.) Ultimately, I don't have a clear picture of the tradeoffs, so I can't make serious suggestions. It's something that only can be developed during discussions. |
Answers for @markus2330
And where would the value be stored between transformation and
If
You can generate a struct with the code-generation API already. In fact this is widely used across LCDproc already. (It is a bit different, because there is special
An option to disable generation of setters may also be useful for other things, e.g. if you intentionally want to prohibit the application from setting keys. Of course just disabling the generation doesn't actually prohibit the application from setting values (there are other ways), but I reduces the likelihood of mistakes. Disabling the generation of the Answers for @haraldg
AFAIK if debugging is not enabled,
I am certainly no compiler or optimization expert, but AFAIK object files will always contain all of the code. Only when linking the executable unused functions may be removed.
More efficient storage formats will result in a loss of execution speed.
The idea was that the specification is embedded into the executable the uses it. That way the specification is always exactly what the executable expects. It cannot be modified without also recompiling the executable (or some intense hacking).
I thought about that, but there are two downsides:
Elektra also has a static version, in case
Currently the code-generation API expects exactly one executable per specification, so the question is trivial. However, there is nothing stopping you from not using the generated sudo kdb mount -R noresolver spec.eqd "spec/sw/lcdproc/lcdproc/#0/current" specload "app=/usr/bin/zcat" "app/args=#0" "app/args/#0=/somewhere/spec.eqd.gz"
Not sure how the whole ELF constructor stuff works. That is beyond my knowledge of C.
One thing you need to take into account, is that the specification has to be accessible to My ObservationsI did some tests too and while I didn't use an ARM chip, I could reproduce similar size increases on x86-64. In my case the increases where not as extreme but still ~1.8x for lcdexec. I then simply commented out all the If we then use the All of these tests where done with TL;DR I think the size increases can be mitigated via compiler options and some changes to how the spec is stored. Sadly it seems, the binary size will still increase, despite not using a statically linked config parser. |
@haraldg wrote:
If it is required that the binary must be able to start without installation is not yet answered. I started an issue for prioritization of features in #2779.
I also do not like it. It was surprising for me that it does. @kodebach I am very much interested in your results how the low-level API compares with the high-level API.
Yes, it goes in this direction (especially @kodebach's suggestion to not generate setters). If you are not against such "compile switches" we can discuss further possibilities to reduce the binary size.
Of course we also support to load it on elektraOpen() but then it only works if the application is already installed (the spec needs to be installed and mounted). To have it part of the binary always works, which is very handy for development (you can run the binary from the build directory). In a further away future, something like "strip" might remove the spec during installation (as for installed binaries it might not be needed). To write such a "strip" utility, however, will also be out-of-scope for @kodebach. @kodebach wrote:
Of course, elektraSet would be more expensive. It always needs to update the KeySet and the cache (struct).
If you consider code-generated APIs to be also part of Elektra, then these APIs are not too slow. They can be as fast as access to variables.
Ahh, ok, now I better understand what you wanted to do. So basically our ideas are the same: I would only hide the structs from the users so that they cannot do changes in the struct (which would destroy synchronization to the KeySet).
And we would also loose introspection, access to defaults, ... and everything else which makes the life of admins easier. Validation is only a small part of this. |
There seems to be a huge misconception here. At the moment applications using the code-generation API absolutely and without exception require the execution of a |
IIRC when using at least
Of course there are tradeoffs. However this just isn't true in general.
I have been sloppy. What I meant was something like: Read it during loadConfiguration() from an external file.
Yes, which is one reason why this spec-is-in-the-binary thing is roundabout. Having the spec in a separate file would make things much simpler. And probably also support modular applications nicely.
Should be easy to prevent accidents by generating unique filenames. You could even use checksums to mildly guard against tempering, but I don't recommend it.
That's interessting - I should check how much size the old config parser takes, to get a better idea where we are standing. However keep in mind, that this are gcc specific optimization features and also (not a concern for lcdproc at all) using these optimizations might not be feasable for linking huge applications. (I don't know anything about this topic, but doing optimzations globally instead of per file seems like something, that might scale badly.)
Ultimately, that will be nice to have. But I suggest we first focus how things should be on production systems and the general ABI for spec access. Development features and lcdproc specifics should come later.
Yes, ideally we would select with
You could also just embed a fallback filename instead of the whole spec inside the binary.
This seems impractical. Much easier to do the right thing during code generation. To explore possible approaches a bit and in the interest of me getting a clear picture about the tradeoffs: Do I understand this correctly, that, if we don't have the spec available, the application will (or at least could) still run, but will crash (via fatal error callback) when we access a value that is missing in the configuration? This behaviour probably is good for unattended embedded systems. Other systems probably don't care much about validation, but still would like to have defaults for missing configruation items. Those could quite easily supported by a variant of the elektraGet API: Something like For non-embedded systems we clearly want the full spec around. If parsing the spec from a space efficient format is deemed too expensive, you could cache a validated version and only revalidate if timestamps change. But this is not relevant at the moment other then to point out that we don't have to take bad compromises between size and speed. |
There are two parts to this. The KeySet with the default values passed to
Like stated above, you can pass default values to If I understand all the concerns correctly, everything should be more or less covered by these three modes then:
|
@haraldg wrote:
Elektra is designed for external specifications and this is also the normal way in Elektra. In the discussions about
We also support signatures of config files. But the built-in spec is the most easy and safe way to never have an out-of-sync spec. Of course it increases the binary size (more than we thought it would).
I am also interested. Unfortunately, the YAML parser is quite big and needs an external library (yamlcpp). This is also a further decision for you to make (which config file format you want by default).
Yes, you have a better feeling of what LCDproc people want.
Ok, should the option rather be called --works-as-standalone or --minimize-binary-size? Or asked differently: do you have plans to reuse the option also for some other optimizations?
This does not help if it is not installed. And if the spec is installed, it works anyway.
Exactly. It would have no defaults, no validation, no description of the config, ....
We should consider here:
@kodebach wrote:
See above.
@haraldg can you please prioritize these three modes?
I assume for LCDproc we should prepare some installation scripts?
What about some run-time option to tell the application it should not fail for mistakes in the spec (like no spec)? Then we would have only two compile modes (either spec is included or not). @kodebach Did you already try to compress the spec? |
The goal of What you can do however is compile the application and mount this local executable via specload (°). Then you can run the local executable. Now you can simply recompile and run again (without another mounting procedure) and will still have the correct spec (even if you changed it before compiling). (°) If you have an installed version of your executable, you need to unmount its spec and then mount with specload. Alternatively you can manually change the configuration of the specload plugin for your mountpoint to use a different executable path. Sadly there is no
Please don't post contradicting information. As I stated above, the defaults can be given separately from the spec.
Each of the 3 modes serves an entirely different purpose and the implementation of these modes is not really complicated so prioritization isn't really necessary. Instead it would be good to know, if these modes cover everything or if we might need additional modes.
First of all: Right now we use separate KeySets for defaults and spec. I will definitely make the changes necessary, so that we can use the same one for both. This should reduce the binary size quite a bit already. Beyond that there are a few possibilities. If we want to construct the embedded spec using the C API (ksNew, keyNew), it seems there isn't much that we can do easily. As far as I few short experiments showed, using a different API (no varargs, many functions calls; different keyNew API etc.) won't help much. (Disregarding how complicated the implementation would be) Another option would be to embed a single string (or byte array) that encodes whole KeySet and use a plugin to use it. Since we have to send the KeySet in quickdump format for specload, the quickdump format would offer itself for embedding. However, right now quickdump seems to be slightly larger than C code (in the case of For non-embedded specs you can simply take a quickdump file compress it with sudo kdb mount -R noresolver spec.eqd "spec/mountpoint" specload "app=/usr/bin/zcat" "app/args=#0" "app/args/#0=/path/x.eqd.gz" You could also use any other method of compressing the quickdump data as long as there is an executable that can print the uncompressed file to stdout. |
I could not find any documentation about this, can you please point me to it? I only know of one argument in elektraOpen which accepts a specification and I assumed the code-generated API will pass the specification using this parameter and that the application called with --elektra-spec will return exactly this.
Why would you do this?
Thank you. |
First of all I should point out, that I'm not familiar enough with Elektra jargon, to fully understand all your remarks. There might be misunderstandings in what I write. I'll try to be aware of the limits of my understanding, but might miss something. When you say "mounting a specification": Am I correct that this basically means having an entry in a file like /etc/elektra.conf point to the file where the spec is stored except for the case where the spec is embedded in the application and you do some things in loadConfiguration(), that have the same effect without actually touching persistent files?
Basically yes. I still think mode 2.5 (cheap default API) is worth exploring, but see below. Also for the sake of compleness, there might be
However, I also have to point out, that I don't understand the difference between (1) and (2): This would behave the same way, just with the spec stored differently?
I don't see this. You can at compile time (code generation time) store the current spec in a file with a unique name and embed the name (absolute path of course) in the executable. If the spec isn't mounted, the executable falls back to the stored location. No?
We really should have had more discussions earlier, if this was the result. But if this is just a development feature, I don't care that much about size actually. I was under the impression, that production builds would have a similar size. If this is just a development feature, then I don't understand why you bother to dump the spec for external users though?
I don't have a strong opinion. Unless there is something wrong with ini, I'd just stick with it.
I don't believe I have the full picture yet, so can't really say. Also I believe somebody might add more modes to the code generator in the future. So maybe just --mode={full-spec,no-spec,...} or perhaps --spec={include,check,ignore} or something like that?
I assumed this would be cheap to implement in terms of functions already existing to support the existing APIs. If this isn't true, then the idea is much less appealing.
Exactly. Maybe except for the case where people use the API, but decide not to use a spec at all.
Somebody doing such sloppy work might just as well access the wrong key in the first place. ;)
I think I don't really understand where you are getting with this. I don't see a usecase for ignoring mistakes that we actually detected successfully?
I have spent hours reading things and discussing with you two and I still don't really understand what this means. Even Markus seems to get confused at times. I stand by what I said earlier: This concept seems very roundabout to me. How can you hope to ever document this in a way that the average user will understand it? |
Mounting in Elektra is always the same concept because specifications are also stored like configurations. Mounting is the mechanism to describe how Elektra can find the configuration below some key name.
Yes, the file that contains the mountpoints is called /etc/kdb/elektra.ecf by default.
Usually mounting refers to file on the file system but it it is not need to be like this. You can also mount "files" from git, network and what executables output.
I would very much prefer to only have functionality which is needed but the needed functionality works very well. Elektra has already more than enough features to "show off", which tend to rather scare away people because not everything works as expected. In particular, Elektra already has features to make configuration values read-only. Having multiple ways to do the same thing need especially good justification. Improved performance could be such a justification but at the moment we do not know anything about run-time in LCDproc yet.
You should also consider, that blowing up a build system with too many option introduces complexity. But yes, an option to only include what is needed for the guarantees (type+defaults) makes sense.
Of course you can do this but as long LCDproc is not installed, the spec file would not be at this absolute path and LCDproc would fail finding the file. Furthermore, having absolute file names inside the executable has other drawbacks (binary would not be relocateable anymore).
Having the specifications of applications available within Elektra is one of the main goals of Elektra: Only then you can query which values the application will get, provide nice tools and so on and so forth.
The INI parser called "ini" is the one with most features but also with most bugs.
In some early discussions we also said that the API should gently push people towards more modern and robust ways of how to access configuration. Having a spec is a must-have for many reasons. Only in very restricted systems someone should consider to not have a spec at all: in cases where you want no run-time configuration at all.
This would be a developer feature to allow developers to start LCDproc without spec. The idea here was that we reduce the number of compilation variants to two: compiled-in spec and external spec.
The high-level API already has nice documentation: https://www.libelektra.org/tutorials/high-level-api But as you noticed, the recent features of code generation currently do not have such documentation and sometimes also work differently than I would have expected it. Thus I urge to a very small number of use cases and implement and document exactly these 2 or 3 cases. |
Is it a correct summary of your the mounting description that configuration
sources (files, whatever else) can be mounted either globally from elektra.ecf
or process-local from within loadConfiguration()?
Of course you can do this but as long LCDproc is not installed, the spec file would not be at this absolute path and LCDproc would fail finding the file.
I actually suggested to store the path of the not-installed location. Ie the
path in the build directory.
Furthermore, having absolute file names inside the executable has other drawbacks (binary would not be relocateable anymore).
Yes, this would only be a development feature, to be able to run non-installed
versions of the application. Production builds probably shouldn't include
this path at all or at least be happy to ignore it.
> If this is just a development feature, then I don't understand why you bother to dump the spec for external users though?
Having the specifications of applications available within Elektra is one of the main goals of Elektra: Only then you can query which values the application will get, provide nice tools and so on and so forth.
I fail to see how referencing the correct not-installed executable in whatever
tools you are thinking about is easier then referencing the correct
not-installed specification.
> I don't have a strong opinion. Unless there is something wrong with ini, I'd just stick with it.
The INI parser called "ini" is the one with most features but also with most bugs.
I have no idea which INI parsers are available. My only statement was about
sticking with INI seems reasonable. But I suspect most people wouldn't care
much about which human readable format is used.
> I think I don't really understand where you are getting with this. I don't see a usecase for ignoring mistakes that we actually detected successfully?
This would be a developer feature to allow developers to start LCDproc without spec. The idea here was that we reduce the number of compilation variants to two: compiled-in spec and external spec.
So basically this would mean we always have to run elektrified applicatons
on openwrt with some command line switch, that nobody uses on non-embedded
systems? People will forget this in their scripts more often then they
make mistakes in their configuration. Seems like a poor tradeoff.
|
There is some mounting-functionality in the APIs (kdbEnsure) but it is not to be used to mount configuration files but only process-local configs like command-line arguments. The reason not to use it, that any non-global mounting would destroy the correct view of tools.
This would then only be interesting for developers where we can add the whole spec anyway.
I am talking about Elektra's tools (kdb, qt-gui, web-ui, ...). Of course developers can also mount the files of their build directories. Unforunately, you need to be root for mounting because mounting writes to /etc/kdb/elektra.ecf. See #1074 for a proposal to change this.
No, the other way round: developers would need to add the switch when they get the error that the specification is missing. But let us first focus on the use cases we need to support, and then decide how to implement them. |
Ok there is a lot to unpack here.
Mounting in Elektra is very similar to mounting a file system in UNIX. Elektra has a system wide configuration of mountpoints. These mountpoints are just keys that defined that everything below this keys (and not below another mountpoint) is loaded using the storage plugin for this mountpoint. So for example you can mount the key UNIX has synthetic file systems like So if you "mount a specification" you will modify the system wide configuration of mountpoints to tell Elektra how to retrieve your specification. This cannot be done inside
I also thought of this as a future possibility. It could be useful for the unattended embedded systems you mentioned earlier. Basically one could play around with the specification and different configurations. Once the final configuration is found a dummy
Yes, I would say that should be done via a different tool. The tool would pre-process the specification before it is passed to the generator.
Yes, the binary would be smaller. The external spec could also be compress in other ways then is easily possible inside the binary. In addition you could store binary and spec in different places. For example the binary could be compiled into a firmware image, while the spec is stored on some other bigger flash chip not intended for executable code. No idea, if such devices exist. Basically (2) is a bit more flexible. While (1) guarantees that binary and spec are always compatible.
Of course it is just a bit of copy/paste. But as soon as there are different options for doing things, you have to be very explicit about the trade-offs between the different options, otherwise people will become confused and might be deterred from using Elektra. One goal of the high-level API was to make it easier for beginners to use Elektra, as the low-level API is quite complex and unintuitive at times.
Like I said above, it is already possible to use the highlevel API without a full spec. The
It absolutely isn't. Again (right now) the only real benefit of using @haraldg you can probably ignore the stuff below, it will contain a lot of Elektra terminology and basically talks about implementation details
There is no real documentation, but looking at a file created by the code-generator might help. Like you said there is the It is still true, that the However, the And finally, just to clarify this once and for all, PS. I have no idea, how the new cache plays into this. It might just work or it might break everything. (I suspect the latter.) Creating and removing mountpoints in (°) AFAIK the |
Thank you for the detailed answer!
Better to only support (1) then?
This is definitely an important thing to be changed. Did you consider that the code generated API looks identical to the non-code generated API? This would deduplicate a lot of the documentation. Ideally, we would simply describe one way to use the high-level API and without code-generation you simply include a different (non-generated) elektra.h file.
Perfect.
Can you create an issue/proposal about these problems? For me the expected behavior is that the spec plugin copies the metadata so that it is available for all plugins. Ideally, keys passed to kdbGet are identical to keys retrieved. (Obviously not for cache hits or NO_UPDATE where kdbGet essentially is a no-op.) |
In what way? I don't see any part that is really identical. A lot is similar, since one is based on the other, but nothing is really identical.
Once I know what I am proposing I can do that...
That is exactly what happens, there are no other plugins, if no mountpoint exists.
Not sure what you mean by that. Documentation for |
Especially in terms of the API, e.g. to have elektraOpen and not loadConfiguration. The "default" parameter would simply be unused or can even be another KeySet that is appended. Are there any other differences in the generated vs. non-generated API? (Except of additional guarantees in the generated API.) |
@kodebach: Thanks for the detailed explanation. It helps a lot, it also raises a lot of new "Why are you people doing things the way you do?" questions. They probably are not useful at the moment, so I think I have only one remaining question: Is it correct to say, that (1) ensures that the application validates it's own configuration against the correct spec even if the wrong or no spec is mounted? |
This is not necessarily true: there are many concepts in Elektra you do not need to understand as long as you are not confronted with them. In use-case 1. you would not be confronted with mounting and spec at all. In general Elektra is carefully designed that distributions can already decide a lot for the users (e.g. mounting all files in some config format). Then users can use the distribution in very similar ways than they use distributions now (only that configuration is unified). They would not, however, need to understand mounting. (That is also the reason why mounting is only possible for root: the feature is meant to be for distributions or
If you see we should not support/document it, we will happily do so. It means that installing LCDproc will be required for starting it. I would find that very annoying.
specload and kdbEnsure would not be used here. We would simply lose most of the guarantees, as no validation would happen. This would be only for developing LCDproc without changing spec or conf. But imho this is also a valid use case. If you do not think so, we will not support it.
Of course this is always a dream but the reality often is that if you mess up in configuration design, users will find many ways to make mistakes. And exposing decisions that could be done by developers is very smelly configuration design.
Then we should do so.
You assume that everyone reads and understands the docu. In fact only people that are as deeply involved as we are now, can really decide which way of storing the specification is better. So it is our responsibility to decide that properly. Pushing this decision to users is not the way to do it.
Yes, I would also not recommend to do it that way. But it hackers will find out and use this trick locally. Hopefully @haraldg will not merge this hack 😄 |
End users (e.g. people using LCDproc) should never be confronted with mounting, completely independent of the code generation modes. The mounting should always be automated in the install process. Developers (e.g. people working on LCDproc) always have to understand mounting, unless there are serious changes to how Elektra works. You cannot develop an application using the code-generation API without knowing about mounting, simply because you have to create the install scripts responsible for mounting an possible have to mount manually during development.
Dropping the built-in spec only requires a different way of mounting. You can still run LCDproc from any directory and and a valid mounted spec is required in both cases.
That is why we need to decide good defaults, not why we shouldn't have options. Also if a developer uses an API without properly studying the documentation, they shouldn't expect to get the best possible results. Since this whole discussion is leading us nowhere, I will now start the implementation. Once I have some code you can try out, we can still decide what do keep, what to change and what to add. |
I mostly agree with @kodebach in the discussion above.
Yes, it is very annoying, but it seems to be beyond the scope of what @kodebach is doing, so we kind of have to live with it. Also it seems to tear at the foundation of elektra to allow any non-global configuration like what the
I don't remember ever running any of the programms from LCDproc with complete default configuration. Maybe I did at some occasion for some very trivial testing, however I have 5 different variants von LCDd.conf (designed to test different aspects) in my build directory, that I use with
What get's merged largely depends on people explaining their use case and convincing me, that it does more good than harm. If people sometimes need to run an application without a spec (say it is on a filesystem, that isn't always mounted), but still would like a warning when the spec isn't available, then I probably shouldn't tell them to "just use the OpenWRT mode, where the spec is never checked" but allow them, to add a switch to the build system, that makes the missing spec less fatal. |
If
I switch in the build system would be an acceptable solution (that switch could just disable the check). What I think we shouldn't implement (unless there is absolute need for it), is to generate the check, but then allow the application to continue even if the check failed. IMO if the check is generated, it has to succeed for the application to work. |
[ Yes, I'm sure everybody will find some workaround that will be acceptable to them.
So you think a warning is worse then no check at all? (BTW this branch of the discussion wasn't about what you should implement, but what I might feel tempted to merge, if somebody proposes it for LCDproc.) |
We make the scripts now for LCDproc. And adding some
This is a general misconception: it is not possible to make configuration sane just by providing good defaults. At the moment you need to change something, you are confronted with the complexity of the configuration. Thus you need to be always aware of two things:
Yes, APIs are an excellent example! Would you like to have an API where you have calls that decide about some internals you have no clue about?
Yes, as already wished several times: please make the necessary fixes so that the clients successfully start, make a PR at the LCDproc repo and then write to the mailing list. Further code-generator-modes and optimization of the binary size should be done after we get further feedback.
The problem with continuation on spec errors is that maybe also other errors are present that are not reported (we always report only one error). |
You missed the point. To develop an application using the code-generation API you need to know about mounting. More specifically, the person responsible for creating install scripts that do the mounting has to know about the concept of mounting. In the case of LCDproc this person is me. I know about mounting, but I won't write the scripts for all project that want to use the code-generation API. The main point was that you left the impression that there is a solution where we completely abstract over the concept of mounting. This is not the case.
Of course simplicity is also important. But if you focus too much on simplicity, you loose flexibility. At some point you have to sacrifice simplicity, if you want to provide a general solution. Elektra wants to provide a general solution, so there has to be some complexity.
Isn't that the whole point of having a high-level API, or any API for that matter? It hides the internals of lower levels, so that you don't have to know about all the complicated stuff. Also this kind of contradicts your previous statement. If the API doesn't decide for me, it has to expose all the options. Therefore it cannot be simple. So do you want it simple or do you want the options?
I already stated a few times, that the clients (and also the server) should start successfully already. I also provided instructions in #2748 (comment). If it doesn't work for you, please let me know. |
Not yet but maybe you find a way to write reusable install scripts? Something like: magic.sh path-to-spec-file application-root-key-name What a the two arguments are, developers obviously need to know.
If someone wants some feature, then he/she of course need to dive into some complexity, e.g. reading about some concepts or plugins (and we are willing to provide also complex features). And you did wonderful work for many different features. But these concepts and plugins should be as simple as possible and do only what is needed. We already have enough monsters (:wave: ini, spec, list plugins; the old code generator). We need to get rid of these monsters and not introduce new ones. Thus Elektra has a very clear quality goal: Prefer simplicity over robustness and extensibility (flexibility). This is stated very clearly in GOALS.md. And we also live this: it is still possible (and always will be) to get/set values in Elektra without using any of these concepts we talk here about. Harald asked about 5 different modes (which I already find over-the-top, but ok, he is the customer) but we should not introduce 12 nobody asked for. In particular, I still do not understand what any user should do with the "defaultsHandling" option. It seems to me a typical "I need an
@haraldg tested it already (see some comments below your link) and he said:
Furthermore, instructions should not be hidden in some discussion. Please wrap everything up and make a proper PR for the main LCDproc repo which also includes a tutorial (for instructions). Thank you! |
Update progress in #2805
I don't have access to an
The For both DH and SL currently the default is To reduce binary size, but still allow running the application without the need for If you want minimal binary size you should set SL to The last combination
I didn't give proper instructions yet, because they might have changed massively after this discussion. I will now start working on the documentation as well.
After adding the different modes and some more testing and experimenting this week, I am now firmly of the opinion that this is not a good idea. Of course the feedback would be nice, but I don't see a way that this PR will be merged in the near future (=this summer). The closer I get to finishing my work, the more problems in other parts of Elektra find. Many of these problems are systemic and obviously arise from the fact that many of Elektras features are hacks. Hacks meaning parts that use the core in ways it was never designed for. Most prominently these are the For example today I found this problem. I have no idea, why it didn't come up until now, or why it even is a problem, but here we are: kdb set -N user /sw/lcdproc/lcdproc/#0/current/lcdproc/foreground true
#> Using name user/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground
#> Set string to "true"
kdb get /sw/lcdproc/lcdproc/#0/current/lcdproc/foreground
#> Sorry, module type issued the error 52:
#> error in the type plugin: The key user/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground was already normalized by a different plugin! Please ensure that there is only one plugin active that will normalize this key.
kdb get user/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground
#> 1 For context, the type plugin is the only one mounted at this mountpoint that does normalization. It is also not a problem with For another problem see #2806. At this point, the only way forward I see is that I create a version of LCDproc that is usable enough to do benchmarks for my thesis. After that I think we need to at least wait for the new plugin system (needed anyway for a proper server implementation without workarounds). |
kodebach writes:
I don't have access to an `armhf` device, so I cannot reproduce this. From what little information was given in the original comment, I suspect that the `gopts` plugin wasn't found.
I guess I should at least have included a full BT for the segfault.
Anyway, I think it is very unlikely, that the problem is armhf specific.
(Especially since kdb works fine.) Compiler bugs are a possibility, but
it is much more likely, that you just have some uncommitted changes in
your working copy, that are actually needed for things to work.
That was my assumption anyway. If I can do anything to gather information,
that helps you pin down the problem, then please let me know.
|
Unlikely. You can try again with the code from #2805. I just tried with that version and starting and connecting the clients did work. You can also check the CMake output for any messages regarding If you get sefaults again, a full backtrace would also be helpful.
|
One reason more to not export this interface to the user.
Another reason. I do not know why you are so reluctant in making this interface easier to use. If it is because you already implemented everything, then you could simply expose only one option (let us say specificationStrategy) and from this option you derive the 3 other options.
Feedback is not nice but essential for the success of the LCDproc elektrification.
This is a completely different matter that is not controlled by you.
Good, please create issues.
Please report this issue (even if you cannot reproduce it yet). For LCDproc this issue is irrelevant as we decided that we will not care about validation. Every attempt to set a boolean to something else than "1" or "0" is not supported. I do not think that LCDproc people will have problems understanding "1" or "0".
No, the only way forward is to reduce to a set that works. We already decided do exclude validation and as next step we could exclude mmap. I hope it is not necessary as we have someone actively working on mmap. Let us see in #2806.
The changes in the plugin system are only related to the maximum number of plugins. It will not magically fix bugs. |
User as in user of the code-generator or user as in user of the application (e.g. LCDproc user)? It will only be exported to the former not the latter. And not exporting it to the former means limiting the use cases of the code-generator needlessly.
That is exactly what I don't want to do, because this is not very transparent. It is also not future proof. If we use the specification for new things, do we add more strategies? Do all the old strategies have the same behaviour or not? With separate options we don't have to think about any of this. It is very clear what the 3 options do. The developer has to decide what is best for their specific use case.
I am sorry but that is not good. It is catastrophic. I created enough issues already to know, they won't be fixed any time soon, unless I do it myself or somebody is actively working on the broken part already.
That's another problem, because "not supported" actually means "you must not do it, even though it works" in this case. I can't say right now how or when exactly, but there are definitely cases where a
No, but they certainly won't like that the version using an actual configuration framework can't do what the old minimalist handwritten parser could do. They also won't like that all numbers (even those that make no sense in decimal like USB Vendor IDs) have to be in decimal, because
But the feedback doesn't help for the problems inside Elektra itself.
It was decided that validation is not required by LCDproc people. But removing validation and conversions is not a good idea. I know @haraldg said he doesn't care about that. But (assuming Elektra's validation actually works) having a spec actually greatly simplifies the code reading the configuration. For example by limiting the range of an integer in the spec you can eliminate the If we give up on validation through Elektra, I have to put all the code back in. Apart from that my main point was: Creating a PR in the LCDproc repo and using a mergable/merged version in my thesis, means involving even more people into this discussion. And that means even more delay until I have a version with which I can do benchmarks. |
It is definitely not needlessly: we found out that many (most?) combinations do not make sense. It is very much needed to restrict such configurations.
It is very transparent if you document the strategies. And this will be needed anyway.
It is unlikely that anyone will want a further strategy in the near future. Harald basically already wants all of them that make sense. It is much more likely, that some of the strategies will rarely needed and bring maintenance headaches.
So you suggest: let us push the responsibility to someone else? I am completely happy with two strategies: the one you implemented up to now and another one to make Harald happy (minimize binary size). I am not happy with the thinking "ohh, we are so flexible, so it must be the users fault when something is messed up".
Of course it is good that you find it. It will be fixed by the person who implements validation (nobody is working on this right now). Validation is a no-goal now. You can be assured it is a long term goal for me.
Of course, that is the status-quo of nearly all FLOSS apps out there. If you mess up the config it might not work, might crash, might format the hard-drive. Elektra is there to change that but one step at a time. We at least guarantee that we do not crash or format the hard-drive. We could give a better error message thus please report the bug.
This is your opinion, which I value very much. And you do your best that everything works very good in your opinion. But we are also interested in the LCDproc people's opinions. At the moment we did not ask them what they actually need, which is not a good situation.
Of course it helps: we'll see to it that Elektra evolves. But without feedback we might run into wrong directions. Especially the validation and transformation problems are very domain-specific.
Exactly!
No, we simply assume everything is validated for now so that you can finish your thesis. In later work we will of course fix the problems.
Ok, this is what it is all about: Do not worry about that. Your grade on your thesis will definitely not depend on whether the LCDproc people like it or not. I like your work except when I say I don't.
For your thesis you can pick any version you like. It will not be the latest version, this never works out so do not try to do that. Some automation for benchmarking might be nevertheless useful, sometimes it is necessary to repeat everything, even without changes in the code. |
1 out of 4 is neither many nor most.
Why? What is the benefit of disallowing certain configurations? If somebody has a use-case we didn't think of why prohibit it?
Exactly why strategies are a bad idea. Separate options are much easier to maintain, since each of them affects much less of the code. That is the basic principle of "separation of concerns".
So if X comes along and wants a performance optimized version we add another strategy? And if Y wants some other version we add that as well?
Of course not. But your version is basically "if you do want we say it works; if that doesn't fit your use case that's not our fault". Also I can't see how anything could be messed up here at all. There are different prerequisites for the different combinations to work, but that is still the case if everything is controlled by one option and there are less possibilities. We already said that there is no magic solution that just works. |
https://cseweb.ucsd.edu/~tixu/papers/fse15.pdf
https://en.wikipedia.org/wiki/Separation_of_concerns Hint: the term is about code and not (configuration) data.
This is how software development works. You add support for another use case you originally did not think of if somebody needs it. Then it makes sense to make it configurable so that for others their old setup still works (so in this case: adding another strategy). As you know software development is not: "let us add random stuff in the case someone might need it". Unfortunately, at the moment it is not widely known that the same is also true for configuration. |
The paper talks about applications with hundreds of options. We are talking about the different between having one parameter with 2-3 values and two parameters with 2 values each. I don't want to go into further discussion (IMO the paper supports my version at least as well as yours), so what strategies do you propose? I think we at least need
Possibly also: And at that point it doesn't really matter that there is a fourth setting, if it makes the documentation easier to understand. In the end the most important thing is that users of the code-generator understand the options. Maybe this is just me, but if each option just controls a single thing (one for spec, one for defaults) that is easier than a single option controlling both.
Yes and how the code-generator works is about code as well. It has nothing do with configuration or data.
It is about the balance between fulfilling exactly the use cases you have right now and making a piece of software that is generally useful. Elektra wants to be useful in general not for one specific case (e.g. embedded system, or server systems). Therefore we should have some flexibility from the start. More importantly software should always be designed in a future proof way. Too many limitations and future changes may become very difficult (e.g. Elektra's limit on the number of plugins is, so deeply ingrained in Elektra that is hard to change now). |
In total Elektra has hundreds of options.
I am okay with all strategies:
This is is not the only point, see above. E.g. see spec, list or csvstorage. They have only understandable options but still they are hardly usable as it is so easy to get these options wrong because most options are usually not needed and depend on each other in surprising ways.
Yes, it was wrong to limit the number of plugins. |
Yes, enabling gopts in cmake and recompiling libelektra fixed the segfault. Thanks for your help. Wouldn't have figured that out on my own easily.
Yes, the number of lines removed from LCDproc was one of the benchmarks we decided upon. However currently LCDproc is very inconsistent with how it checks or fails when there is a configuration error. We don't put much thought into it, ergo it is not important.
No, please don't. As Markus said: Just assume libelektra will be fixed in time. I can't merge anything before it does anyway.
I doubt people will be adding much to the discussion unless they have a really strong opinion on something. OTOH I believe it would help a lot for the eventual acceptance of elektra, if people feel involved a bit. And getting feedback of the kind of "this is never going to work for my use case" is something we really want to get early, if it exists. However if you feel the current state of things is too embarrassing to show to the public, then this is your call to make. I respect that. But I still hope that the issues with number of plugins etc mostly affect LCDd and we can show at least some reasonably working client around soon. It need not be a full PR and it even need not be in the LCDproc repo (though why not?). But having some branch of libelektra and some branch of your LCDproc fork, that are confirmed working together and won't be changed during your ongoing development work, would help interested parties a lot.
Out of curiosity: What benchmarks do you intend to do? About this whole "which way to present code generator options to users" discussion: I think this is a very minor UI concern, that really isn't important. What ever you decide there will be much less anoying for the casual developer, then other things like Elektra inventing its own data types for example. Personally I'd treat this like gcc treats optimization options: All the world knows that they can chose beween Og, Os, O2 and O3, but all the optimizations are still documented individually and can be selected on the command line if you really want to. This seems a good compromise between usability, precise specification of behaviour and interface stability - nobody expects all the optimization features to stay stable between gcc releases, but of course all the world relies on the meaning of Os and so on staying the same. So maybe there really should be all the technical options, that @kodebach wants, but also a stategy option like @markus2330 believes is more usable and would actually be the stable interface. |
Good that this is fixed. Did you somehow compiled in an unclean state or is this something we need to fix? In any case, it should segfault but report that some library is not missing, shouldn't it?
Exactly. And config formats often is something, people have very strong opinions about.
I think so, too. This was also very clear from the previous email about Elektra to the mailing list.
Of course every individual unnecessary option is not important but once you have hundreds it becomes a big problem.
From type theory point of view Elektra does not invent any new types. (In C struct makes new types but typedef is only an alias to existing types). Practically, typedefs can be annoying. If you want, we can rename all types in LCDproc to the C99 types. (Maybe in a later stage.) As on C99 platforms we hopefully always pick the C99 types (if not it would be a bug).
If you need the low-level options to play around with optimizations we can of course offer them. But a big warning: as I understand them they do not only tune performance but also sacrifice correctness or introduce different behavior, something most gcc optimization options don't do. |
markus2330 writes:
Good that this is fixed. Did you somehow compiled in an unclean state or is this something we need to fix?
I compiled elektra from a fresh git clone a few hours before @kodebach
wrote his instructions. So instead of copying his command lines, I just
did a plane cmake ...; make; make install sequence.
It seems the gopts isn't built by default yet. Whether this is intentional or
an error is up to you.
Practically, typedefs can be annoying.
They make the code harder to read for casual users of elektra.
If you want, we can rename all types in LCDproc to the C99 types.
Yes, I'd prefer that non-standard typedefs leak into LCDproc code as little
as possible.
In this particular case the ship probably has already sailed, but I
believe using non-standard types in the Elektra API is a poor choice
too: It just confuses people.
If you need the low-level options to play around with optimizations we can of course offer them. But a big warning: as I understand them they do not only tune performance but also sacrifice correctness or introduce different behavior, something most gcc optimization options don't do.
I certainly don't need them, but I might find them nice to play around with.
However my main point was: The points raised by you and @kodebach both
have their merits, so it might make sense to follow both: You get your
stable interface and @kodebach gets his options with clearly defined
behavior.
Yes, that code generator options will result in different behaviour (at
least on error paths) is clear. gcc options are just an analogon, not
completely equivalent.
|
It will be included by default once it is not experimental anymore.
Yes, I agree. Is it okay for you to have something like
In the low-level API we do not have this problem, as the API user needs to do the conversion. In the internal-notification API we actually have both: C types and an API for our typedefs. If we change to C99 types, we should do this consistently in both internal-notification and highlevel. Would be quite an effort but it could be done without breaking compatibility (as typedefs are different names for the same and as such they must be compatible). Another question is if the high-level API (and the internal-notification) is C-only or also for other languages. Iirc @kodebach also had in mind that the high-level API should be easy to write bindings for. |
Is it okay for you to have something like `int32_t x = elektraGetLong(...`.
Sure. As long as it is clear on first glance what is going on, it's fine.
|
Switching from the
Under C99+ the change should be quite simple, but at least parts of Elektra work without full C99 as well. Thats why If we require C99+ for all of Elektra, we should also switch to using C99 types IMO.
Bindings don't really play a role here. Other languages have different types anyways. If anything bindings would be a point in favor of using C99 types, because those might be automatically mapped to standard language types. |
Yes. @kodebach Did you try if it actually works? (With a compiler that has no understanding of C99 like VisualC++?) If it does not work, we should definitely get rid of all the complications there and simply use (only) C99 types.
Yes, compiling Elektra needs C99 for a long time but compiling Software using Elektra (at the moment) might still work with pre-C99 (if all the From communication point of view it we definitely should start saying "long" is a signed 32 bit integer and stop pointing to the CORBA standard (it only scares people). At least internally, "src/include/kdbtypes.h" makes sense anyway (for checking the availability and to define the printf specifier). But I see that applications do not want to use this file. Btw. We already have an open issue about unifying the type system: #1092 |
Thank you for the discussion, please open new issues for further discussions. |
Hi,
I just have looked at a few samples of the LCDproc code you wrote and in extension the highlevel API for the first time. And with "looked" I mean just looked: Hadn't time yet to setup a container where I can safely build elektra master and try a few things for real or check the benchmarks we defined at the start.
The first thing to notice is, that the code is very verbose. While tokens like ELEKTRA_TAG_LCDEXEC_ADDRESS are easy to understand on first glance, I find that they make the code harder to read if you have many lines of this.
Next you seem to love opaque data types. I'm aware of the benefits for keeping the ABI stable while things change under the surface. However the downside is, that this makes the code again harder to read in huge quantities and also bloats the resulting binaries a lot. (Each call to an external getter typically takes at least 3 instructions and invalidates the contents of all registers. From a green programming POV that's rather unfortunate.) The latter problem can be "fixed" by making the getter inlineable, but of course at the cost of all the benefits. I'd reconsider which data types are actually worth being opaque.
I see that you are providing your own versions of basic data types like long. What problem are you trying to solve with that? IMO it mostly makes the code more difficult to read.
In particular I dislike, that you also used elektra data types in LCDproc data structures not directly related to configuration.
That error handles are malloc()ed data structures is rather surprising. Especially because I remember we had a discussion about a year ago, where I advocated treating malloc() as fail safe and you disagreed. Now you are using malloc in a context where you clearly have to treat it as fail safe. There are only a few functions, that actually can cause errors, so this is a rather unimportant topic, but I find the choice rather unorthodox. Most people seem to try to do error handling on the stack.
Maybe this is already answered in some document, but I didn't find it quickly: When elektraGet() returns a string (or other pointer to a data structure), what's the scope where the object is valid? My guess is until elektraClose() is called, but it's not obvious. Also what's a good policy for keeping the Elektra handle around with regards to resource usage (and other considerations)?
I see that you sticked pretty close to the original structure to the code. I guess for some of your goals this makes sense, but OTOH I wonder if you can really show off elektra while sticking to the structure dictated by a foreign configuration API. Maybe you can share your thoughts on the topic. Also maybe it is interesting for the thesis to compare for one driver the difference between a straight translation and an idiomatic elektra implementation, if there is any.
Sorry if this sounds mostly negativ. I suppose it's natural, that, when looking at something very quickly, mostly the negative things stick out. I will try to soon find the time to explore your code in more detail.
HTH,
Harald
The text was updated successfully, but these errors were encountered: