Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Hex Support #1388

Closed
domhof opened this issue Feb 26, 2017 · 10 comments
Closed

Hex Support #1388

domhof opened this issue Feb 26, 2017 · 10 comments

Comments

@domhof
Copy link
Contributor

domhof commented Feb 26, 2017

In view of the kdb-types defined in kdbtypes.h we can implement the following getters in KDBHL:

int kdbhlGetInt (KDBHL * kdbhl, const char * name);
const * char kdbhlGetString (KDBHL * kdbhl, const char * name);
kdb_boolean_t kdbhlGetBoolean (KDBHL * kdbhl, const char * name);
kdb_char_t kdbhlGetChar (KDBHL * kdbhl, const char * name);
kdb_octet_t kdbhlGetOctet (KDBHL * kdbhl, const char * name);
kdb_short_t kdbhlGetShort (KDBHL * kdbhl, const char * name);
kdb_unsigned_short_t kdbhlGetUnsignedShort (KDBHL * kdbhl, const char * name);
kdb_long_t kdbhlGetLong (KDBHL * kdbhl, const char * name);
kdb_unsigned_long_t kdbhlGetUnsignedLong (KDBHL * kdbhl, const char * name);
kdb_long_long_t kdbhlGetLongLong (KDBHL * kdbhl, const char * name);
kdb_unsigned_long_long_t kdbhlGetUnsignedLongLong (KDBHL * kdbhl, const char * name);
kdb_float_t kdbhlGetFloat (KDBHL * kdbhl, const char * name);
kdb_double_t kdbhlGetDouble (KDBHL * kdbhl, const char * name);
kdb_long_double_t kdbhlGetLongDouble (KDBHL * kdbhl, const char * name);

@tom-wa Is there anything else you would need or like to have?

@markus2330
Copy link
Contributor

If we keep using CORBA I would argue we should also use them exactly as specified and not introduce one or two extras ;)

@tom-wa usually does not see when he is mentioned, so you might need to send him an extra email. (He is subscribed to all changes.)

Btw. did you already look into doc/todo/FUTURE? Names ending with _t seem to be reserved by POSIX. We should clarify this before letting the types leak into the API.

And if we use them, please make a pedantic review of the types if they really conform to what they claim to be.

@tom-wa
Copy link
Contributor

tom-wa commented Feb 28, 2017

i'll update my email filters..
i'm missing 2 things (they are not corba though so i'm not sure if they actually belong here): void * and unsigned char (because: compilers)

@markus2330
Copy link
Contributor

As already mentioned I would say we should take a deeper look into the CORBA spec.
I am all for making the types more strict, with known ranges and sign.

So kdb_char_t should be signed or unsigned, but not unspecified.

Sharing configs is important, so we should really have precise specifications of ranges and serialization.

void *

Do we really want binary stuff in a high-level API? I see that we should support it (crypto, compression,..) but it is not intended to be directly used by applications.

Anyway, I am completely missing the discussion which is the absolute minimal API that supports lcdproc that we should start with.

It is easy to extend the API, but it is impossible to get rid of mistakes (like sign of char).

@tom-wa
Copy link
Contributor

tom-wa commented Feb 28, 2017

from what i get from the CORBA 3.3 spec paragraph 3.11.1.3 Char Type it's not defined if char should be signed or unsigned.
most other sources (different lecture materials, books on corba, etc..) state that a char is neither signed nor unsigned and a value from 0-255

since the signedness of char in c is also implementation defined and some implementations have issues when passing a signed char i'd say kdb_char_t should be unsigned.

Do we really want binary stuff in a high-level API? I see that we should support it (crypto, compression,..) but it is not intended to be directly used by applications.

not sure if there are many (any?) relevant usecases where reading/storing binary data is needed but i'd rather have it supported but hardly ever used than not support it at all.

Anyway, I am completely missing the discussion which is the absolute minimal API that supports lcdproc that we should start with.

const char *, kdb_long_t, kdb_boolean_t, kdb_double_t would be enough assuming the integer functions also work with hexadecimal strings

lcdproc also uses tristate logic for some values, but i guess that belongs somewhere else

@markus2330
Copy link
Contributor

it's not defined

I would argue that we should specify it for us anyway. Strange that they did not say it is "implementation defined". But the 0-255 is quite specific anyway.

kdb_char_t should be unsigned

Yes, fully agree.

not sure if there are many (any?) relevant usecases where reading/storing binary data is needed but i'd rather have it supported but hardly ever used than not support it at all.

It is supported via the lower-level API. Without any good usecase I am against having it supported it in the high-level API. For obscure cases, users can always fall back to the lower-level API (even if they use the higher-level API otherwise).

hexadecimal strings

Do you mean strings encoded as hexadecimal chars, or a number encoded as hexadecimal string?

In the first case, we have the hexcode plugin. (Needs more testing though).

In the second case, we need another (trivial) transformation plugin.

lcdproc also uses tristate logic for some values, but i guess that belongs somewhere else

In the code generator it is straightforward: The user would get an enum. Otherwise the user needs to cast the long to the enum himself.

And seems like we also need a plugin to transform enums to/from integers.

@tom-wa
Copy link
Contributor

tom-wa commented Mar 1, 2017

Do you mean strings encoded as hexadecimal chars, or a number encoded as hexadecimal string?

the latter.
is there any reason not to use strtol with base 0 in kdbhlGetLong?

@markus2330
Copy link
Contributor

Yes, unfortunately there is a catch. Doing so that would mean that 0.. values always have a special meaning, which has some implications:

  • the type checker needs to be less strict (novice users might not even know what octal is and just put a leading zero for aesthetic reasons or because they mistyped)
  • setting such auto-values is non-trival (we would need to detect what it has been before within the high-level API, but our goal was to keep the high-level API very thin)
  • Users of lower-level APIs might do it differently and it won't work for them (especially those who do not have strtol available)

How does it work for lcdproc at the moment? Are hexvalues allowed at any place where numbers are allowed?

@markus2330 markus2330 mentioned this issue Mar 1, 2017
6 tasks
@tom-wa
Copy link
Contributor

tom-wa commented Mar 2, 2017

How does it work for lcdproc at the moment? Are hexvalues allowed at any place where numbers are allowed?

lcdprocs config_get_int uses base 0 strtol and some configs use hexadecimal representation of values (eg ports)
we could let integer getters accept a "base" parameter and assume base 10 if no parameter is provided ?

@markus2330
Copy link
Contributor

@domhof Can you make a proposal for a specification of how all data types are represented as string? Does CORBA already provide such a specification?

lcdprocs config_get_int uses base 0 strtol and some configs use hexadecimal representation of values (eg ports)

Thanks for looking it up! So we need to support hexadecimal at all places. (lcdproc can later decide to not allow it everywhere).

we could let integer getters accept a "base" parameter and assume base 10 if no parameter is provided ?

That would be a punch through the abstraction that Elektra should provide. There should not be information how to interpret values within the API, which is not already part of the configuration specification.

I am open to allow 0x in general for every number (would be the easiest solution, but would add unwanted logic to the high-level API). Maybe we need such a logic for floats, too? (Which often also have more than one possible notation for the same number).
The question is where to draw the line, please take a look at #1398 for the bigger picture.

What do you think?

markus2330 pushed a commit that referenced this issue Mar 4, 2017
@markus2330 markus2330 changed the title Datatypes in KDBHL Hex Support Mar 17, 2017
@markus2330 markus2330 mentioned this issue Mar 17, 2017
6 tasks
@markus2330 markus2330 mentioned this issue May 19, 2018
7 tasks
@markus2330
Copy link
Contributor

Closing as this is already implemented in #1903 by @kodebach (API still pending though #1605)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants