Skip to content
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

allow access to KeySet of meta-data #2983

Open
markus2330 opened this issue Sep 19, 2019 · 40 comments

Comments

@markus2330
Copy link
Contributor

commented Sep 19, 2019

When meta-data was introduced it was not so clear which datastructure will be the ideal one for meta-data. As the KeySet now is also a hash-map, the KeySet seems to be the best candidate.

Because of that, it is not needed to hide anymore which datastructure is used for metadata. Thus I propose to directly expose the Meta-KeySet to API users. We would only need to implement one run-time restriction for meta-data keysets: non-meta data keys cannot be added (this implies that all keys inside would be read-only).

Furthermore, the old keyMeta* should be moved to the libmeta library.

Possible advantages:

  • same code to manipulate KeySets and Meta-Data. This is particularly useful for 3-way merges (@Chemin1) or storage plugins (@sanssecours)
  • bindings can reuse iterator logic for KeySet and meta data (@PhilippGackstatter @raphi011)
  • availability for all the methods for keysets (like cutting, serializing)

What do you think?

@markus2330 markus2330 added the proposal label Sep 19, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@kodebach wrote in #2979:

If we return an actual KeySet that would open up the possibility of metakeys having metakeys of their own. This also opens up the possibility of cycles in the references. I don't think this is a good idea...

The keyset would, of course, be read-only. It would only be allowed to add/del new meta-keys. The keys itself are already read-only. So the change within ks* would not be too big. If the meta-functions are moved to libmeta, then the client-code should also run unmodified.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

The keyset would, of course, be read-only. It would only be allowed to add/del new meta-keys.

That's a contradiction... "read-only" != "add / del new meta-keys"

Also, how would this be enforced? I assume through some internal flags. IMO that would be a bad API, since you cannot tell easily tell whether a KeySet can be used with a certain function or not.

If the meta-functions are moved to libmeta, then the client-code should also run unmodified.

Clients would, however, be required to link against libelektra-meta.

IMO we should just add a const char * keyMeta (Key * key, const char * metaName); and KeySet * keyMetaKeySet (Key * key); to libelektra-core. These are pretty essential functions. Their code should also be extremely small, so binary size impact should also be irrelevant.

Ideally keyMetaKeySet would return a const KeySet *, but that is problematic because of the internal iterators.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

So does that mean the old API vanishes completely or not?
If it does, then we'd have a single keyMetaKeySet function that returns the KeySet, and all the CRUD actions would go through that? Then the function really can't return a readonly const KeySet*, otherwise how do we use ksLookup to delete, lookup and change a key or ksAppendKey to create a new metakey?

And we'd have to add some (not sure how much) code to prevent users from adding a key with metakeys to the Meta-KeySet (basically what @kodebach said) or add a Binary Key to the Meta-KeySet and maybe other bad things.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

That's a contradiction... "read-only" != "add / del new meta-keys"

It only seems to be a contradiction. The content can be read-only and still the KeySet can allow add/del. Think of the difference between const * and * const.

Also, how would this be enforced? I assume through some internal flags. IMO that would be a bad API, since you cannot tell easily tell whether a KeySet can be used with a certain function or not.

Yes, it would be an internal flag. But this is an implementation detail. We could add a keyIsMeta and ksIsMeta but I am not sure if this is needed.

Clients would, however, be required to link against libelektra-meta.

Yes, and this would actually much more represent the idea of Elektra: to keep the core as small as possible. The meta-data accessors are actually not essential once the meta-keyset is exposed.

So does that mean the old API vanishes completely or not?

It vanishes from the core but is moved to a separate library. Code using these functions would simply link against libmeta.

The current libmeta needs a cleanup: there is lot of old stuff there.

And we'd have to add some (not sure how much) code to prevent users from adding a key with metakeys to the Meta-KeySet (basically what @kodebach said) or add a Binary Key to the Meta-KeySet and maybe other bad things.

Yes, this would be the implementation task. First meta-keys need to be protected to be:

  • not binary
  • read-only (no changes of name or value after creation, this should already be implemented)

And the meta-keyset would need a protection that nothing except meta-keys are added.

We also would need a way to create a meta-keyset. I propose that KeySet * keyMeta (Key * key, int options); creates the keyset if it did not exist before and options contain KDB_O_CREATE.

@markus2330 markus2330 referenced this issue Sep 20, 2019
0 of 4 tasks complete
@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

The content can be read-only and still the KeySet can allow add/del.

So only the Keys inside the KeySet are read-only, not the actual KeySet itself.

Yes, it would be an internal flag. But this is an implementation detail.

That is my problem. There is no way to tell what is a metadata keyset and what isn't, without look at where the keyset came from.

We could add a typedef KeySet MetaKeySet and use the name MetaKeySet as the return type of the new function. It wouldn't actually provide any compile-time safety of course, but a developer reading the code would see which keysets are for metadata and which aren't.

this would actually much more represent the idea of Elektra: to keep the core as small as possible. The meta-data accessors are actually not essential once the meta-keyset is exposed.

If I'm not mistaken, the new keySetMeta and keyGetMeta would just be (assuming keyMetaData returns the metadata keyset):

const Key * keyGetMeta (Key * key, const char * metaName)
{
    if (key == NULL || metaName == NULL) return NULL;
    return ksLookupByName (keyMetaData (key, KDB_O_CREATE), metaName, 0);
}

ssize_t keySetMeta (Key * key, const char * metaName, const char * metaValue)
{
    if (key == NULL || metaName == NULL) return -1;
    if (metaValue == NULL)
    {
        keyDel (ksLookupByName (keyMetaData (key, KDB_O_CREATE), metaName), KDB_O_POP));
        return 0;
    }
    Key * meta = keyNew (metaName, KEY_META_NAME, KEY_VALUE, metaValue, KEY_END);
    ksAppendKey (keyMetaData (key, KDB_O_CREATE), meta);
    return keyGetValueSize (meta);
}

Keeping the core small is a good thing, but having to link a second library (which has some overhead), because of two tiny functions is bad design IMO.

I propose that KeySet * keyMeta (Key * key, int options); creates the keyset if it did not exist before and options contain KDB_O_CREATE.

I think the function should not be called keyMeta (that would mirror keyString and should return the meta value). Maybe we call it keyMetaData instead.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

We could add a typedef KeySet MetaKeySet and use the name MetaKeySet as the return type of the new function.

Yes, good idea.

If I'm not mistaken, the new keySetMeta and keyGetMeta would just be (assuming keyMetaData returns the metadata keyset):

Yes, like this. They are actually doing the same as now, the only difference is that they would not access private data but would use the new function returning the meta-keyset.

Keeping the core small is a good thing, but having to link a second library (which has some overhead), because of two tiny functions is bad design IMO.

It is 7 and not 2 functions currently in core just for meta-data. They are implemented in src/libs/elektra/keymeta.c which has 537 lines.

But this is not the reason. The actual reason to remove it: Many libraries/applications/tools/plugins do not need it. Working with meta-data is already an advanced feature. And in many use-cases the API with the KeySet is more comfortable then our meta-API.

@markus2330 markus2330 referenced this issue Sep 20, 2019
0 of 11 tasks complete
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

We also would need a way to create a meta-keyset. I propose that KeySet * keyMeta (Key * key, int options); creates the keyset if it did not exist before and options contain KDB_O_CREATE.

When would someone call this function and if it doesn't exist, not want to create a new one?

With this signature, every time a user wants to access the keyset, an additional argument has to be passed: keyMetaData(key, 0). So the majority of calls would look like that, and only one would be with KDB_O_CREATE. It'd be much nicer if it was only keyMetaData(key).
Can we not create the keyset by default if it doesn't exist?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

When would someone call this function and if it doesn't exist, not want to create a new one?

To find out if meta-data exist at the moment. Alternatively, we could also provide a keyHasMeta.

But in general such flags are nice as they allow to extend APIs without any compatibility issues (older versions can simply ignore the flags).

It'd be much nicer if it was only keyMetaData(key).

In programming languages with overloading or default parameters it will look like this anyway.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

To find out if meta-data exist at the moment.

Yes, if (keyMetaData (key, 0) == NULL) is a bit shorter, but if (ksGetSize (keyMetaData (key)) == 0) works just as well.

Also, most of the time you would be calling keyMetaData (key, KDB_O_CREATE), since you actually want the function to create the KeySet. That means the flag would be even more annoying.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

We could also make KDB_O_CREATE the default and give a flag for not creating.

since you actually want the function to create the KeySet

Not if you simply want to iterate over the meta-data (like spec plugins and many others are doing). If we would create keysets in this case, we might create a useless KeySet for every key. And roughly estimating, I am afraid that even an empty KeySet is at least 100 byte.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

How about adding int keySetMetaData (Key * key, const KeySet * metadata) to replace the whole metadata keyset. This would also allow removing the metadata keyset once created.

int keySetMetaData (Key * key, const KeySet * metadata)
{
    if (key == NULL) return -1;
    if (metadata == NULL)
    {
          if (key->meta != NULL)
          {
                ksDel (key->meta);
          }
          key->meta = NULL;
    }
    else if (key->meta == NULL)
    {
        key->meta = ksDup (metadata);
    }
    else if (key->meta != metadata)
    {
        ksCopy (key->meta, metadata);
    }
    return 0;
}

EDIT: The ksDup line has to be modified to mark the KeySet as a metadata keyset.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Nice idea, but with this implementation you could not get the meta-data? What about:

KeySet * keyMeta (Key * key, const KeySet * metadata)
{
    if (key == NULL) return -1;
    if (metadata == NULL)
    {
          return key->meta;
    }
    else if (key->meta == NULL)
    {
        key->meta = ksDup (metadata);
    }
    else if (key->meta != metadata)
    {
        ksCopy (key->meta, metadata);
    }
    return key->meta;
}
@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

Nice idea, but with this implementation you could not get the meta-data?

You misunderstood... keySetMetaData (for setting keyset) would be in addition to keyMetaData (for getting keyset).

What about: [...]

With your version, there is no way to remove the metadata keyset.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Ok, but then your implementation has the "problem" that anybody who wants KDB_O_CREATE needs:

KeySet * ks = keyMeta (key);
if (!ks) keySetMeta (key, ks = ksNew(0));
@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

That's why I called it keySetMetaData. That way libmeta can still implement the old keySetMeta, which is sufficient for many use cases.

Also my version of keySetMetaData could also return key->meta on success and NULL on error. (Yes, keySetMetaData (key, NULL) would always return NULL, but that doesn't really matter.) Then the KDB_O_CREATE case would be:

KeySet * ks = keyMetaData (key);
if (ks == NULL) ks = keySetMetaData (key, ksNew(0));
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

keySetMetaData

That would complicate the API in comparison to today though, where we simply have setter and getter for metadata. (If we forget about the specifics of keyGetMeta). Is there a good reason why users should have to manage the meta-keyset of a key in that way?

To find out if meta-data exist at the moment. Alternatively, we could also provide a keyHasMeta.

I like the idea of having a keyHasMetaData and a keyGetMetaData, which creates the keyset if it doesn't exist. No flags, and no need for the user to manage the Meta-KeySet.

But in general such flags are nice as they allow to extend APIs without any compatibility issues (older versions can simply ignore the flags).

Does the current metadata API have some limitation that we could get rid of? Because if not, then we should translate the current API to the new one with the KeySet in the simplest way possible.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@kodebach @Chemin1 can you live with:

int keyHasMeta (Key * key);
KeySet * keyMeta (Key * key);

to get rid of metadata, you need to ksClear(keyMeta(key)); keyHasMeta (key); // -> false. (keyHasMeta will then actually get rid of the KeySet by calling ksDel).

Then built on these two functions we can move following nine functions to libmeta:

int keyRewindMeta(Key *key);
const Key *keyNextMeta(Key *key);
const Key *keyCurrentMeta(const Key *key);

int keyCopyMeta(Key *dest, const Key *source, const char *metaName);
int keyCopyAllMeta(Key *dest, const Key *source);

const Key *keyGetMeta(const Key *key, const char* metaName);
ssize_t    keySetMeta(Key *key, const char* metaName,	const char *newMetaString);

int keyIsBinary(const Key *key);
int keyIsString(const Key *key);

keyIsBinary simply is: ksLookupByName(keyMeta(key), "binary", 0) != 0 but within libmeta we obviously can leave the implementation as-is.

I am very much interested if and how much this makes libelektra smaller.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I like this solution, overall.

But what's wrong with ksDel(keyMeta(key)); to remove metadata?
keyHasMeta should take a const Key* key and not have side effects, ideally.

Also keyIsBinary and keyIsString are part of keytest and not keymeta, why do they move to libmeta?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

But what's wrong with ksDel(keyMeta(key)); to remove metadata?

How can the key know that it's meta-data has been deleted? When someone tries to access it later, he/she will get a pointer to a deleted KeySet. So actually, we need to protect the meta-data keyset from removal.

Also keyIsBinary and keyIsString are part of keytest and not keymeta, why do they move to libmeta?

src/libs/elektra/keytest.c is a file belonging to the core.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

can you live with:

int keyHasMeta (Key * key);
KeySet * keyMeta (Key * key);

I think that is actually a dangerous solution. This way you must check keyHasMeta before calling keyMeta, if you just want to iterate metadata. I think the second parameter to keyMeta might be the best solution after all. However, I don't think we will ever need other flags apart from KDB_O_CREATE, so IMO we should make it a boolean parameter.

KeySet * keyMetaData (Key * key, bool create);
// or if don't want to rely on stdbool.h
KeySet * keyMetaData (Key * key, int create);

I also still think that we should call it keyMetaData to keep the possibility of adding const char * keyMeta (Key * key, const char * metaName); in libmeta.

If keyMetaData(key, false) is called and key->meta is empty, we should delete it and return NULL.

Also keyIsBinary and keyIsString are part of keytest and not keymeta, why do they move to libmeta?

src/libs/elektra/keytest.c is a file belonging to the core.

I agree, keyIsBinary and keyIsString should remain part of the core. From a users perspective, it is not obvious that these functions rely on metadata. In fact it might be better to not rely on metadata to avoid the ksLookup call. Instead we could use a bit in keyflag_t flags.

For backwards compatibility the binary metadata could still be set in keySetBinary.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Alternatively, we could always create a KeySet but make the KeySet not allocate an array by default. (If called by ksNew(0, KS_END)). Then we would not need keyHasMeta.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Even without an array allocating a KeySet is wasteful. sizeof(struct _KeySet) is about 60 Bytes. And the problem isn't even the amount of memory, it's more the amount of unnecessary malloc calls.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Yes, I know. But it seems like we need a trade-off usability vs. memory here. The API that makes everyone happy, needs a bit more memory.

Without internal iterator and without hashmap the KeySet could be about 20 bytes. And nearly all data-structures could be directly embedded (key, value and Meta-KeySet could be directly in Key), it is only that nobody did these optimizations so far (and I also do not see them as a 1.0 goal as it has nothing to do with the API). Actually, ks->cursor seems like something that could be removed with minimal effort.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I'm fine with always allocating a KeySet, modern systems should have enough memory. Thinking about it most keys will have metadata anyway. Most keys without metadata are probably only temporary structures inside some plugin/library.

Nevertheless, there is one optimisation I would say is absolutely necessary. If a key is created with KEY_META_NAME, i.e. it is a metakey. We shouldn't allocate the keyset. For one that actually prevents metakeys from having their own metadata, for another it saves a lot of memory.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Yes, I fully agree!

I do not know if the KEY_META_NAME is the best idea, though. Maybe we should simply put meta-data also in its own name-space meta.

So meta/binary instead of simply binary. (Or meta:/binary if we can do #1037)

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Maybe we should simply put meta-data also in its own name-space meta.

Yes, that might be a good idea. Although it is a huge problem for backwards compatibility.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

During 0.9.* backwards compatibility is not important. Lets try to get the best solution we can get with our time budget.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Although it is a huge problem for backwards compatibility.

Maybe we can continue to use keySetMeta and keyGetMeta like today, by having them prefix the provided name/keyname with meta/, so we could continue to use the API like today. It would also be weird to call keySetMeta("meta/key", "value") even though the "metaness" of the key is implied in the function name.

On the other hand, this might be confusing for new users if they can use the get/set API without the prefix, but have to use it when using the meta keyset directly. I think it's good enough as a temporary solution, but not for 1.0.

@Chemin1

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

... @Chemin1 can you live with: KeySet * keyMeta (Key * key);

This seems like a useful function to me.

keyHasMeta should take a const Key* key and not have side effects, ideally.

I would not expect a function called keyHasMeta to delete something as a side effect either.

I think that is actually a dangerous solution. This way you must check keyHasMeta before calling keyMeta, if you just want to iterate metadata. I think the second parameter to keyMeta might be the best solution after all.

I'd much rather type an additional parameter than having the precondition that keyHasMeta was executed before. Such a condition looks like a lot of potential for errors to me.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Maybe we can continue to use keySetMeta and keyGetMeta like today, by having them prefix the provided name/keyname with meta/, so we could continue to use the API like today.
[...]
On the other hand, this might be confusing for new users if they can use the get/set API without the prefix, but have to use it when using the meta keyset directly.

#ifdef ELEKTRA_META_NAMESPACE_COMPAT
Key * keyGetMetaCompat (Key * key, const char * metaName);
int keySetMetaCompat (Key * key, const char * metaName, const char * metaValue);

#define keyGetMeta keyGetMetaCompat
#define keySetMeta keySetMetaCompat
#else
Key * keyGetMeta (Key * key, const char * metaName);
int keySetMeta (Key * key, const char * metaName, const char * metaValue);
#endif

We could use something like this to the header. The *Compat versions prefix metaName with "meta/", the normal version don't. That way you just have to add a #define ELEKTRA_META_NAMESPACE_COMPAT (or -DELEKTRA_META_NAMESPACE_COMPAT as a compiler argument) to ensure compatibility for old code. ELEKTRA_META_NAMESPACE_COMPAT could then be removed in the next release with breaking changes, giving users more time to update larger code bases.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Maybe we can continue to use keySetMeta and keyGetMeta like today, by having them prefix the provided name/keyname with meta/, so we could continue to use the API like today. It would also be weird to call keySetMeta("meta/key", "value") even though the "metaness" of the key is implied in the function name.

Yes, I agree that keySetMeta/keyGetMeta should automatically add the namespace.

This seems like a useful function to me.

I hope you will use it in your implementation. Merging is one of the main reasons to have meta-data as normal KeySets.

The *Compat versions prefix metaName with "meta/", the normal version don't.

I think we can always prefix it, keySetMeta has no other purpose than to add meta-data.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

So I think we basically agree that:

  • the function to return the KeySet should exist
  • the meta-data functions be moved to libmeta (implemented with the function)
  • we make a new KeySet not allocate the array if it should be used for meta-data
  • we make the meta-keyset know that it is a meta-keyset (and it rejects to add normal keys then)

Open is:

  • how to name the new function: keyMeta, keyMetaData, keyMetaKeySet were proposed
  • how to make the KeySet for meta-data. Maybe to pass two times the same special pointer addresses (which cannot happen on valid ksNew calls)? E.g. ksNew (2, 0xEADAEADA 0xEADAEADA, KS_END)
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

how to name the new function: keyMeta, keyMetaData, keyMetaKeySet were proposed

@kodebach wrote

I think the function should not be called keyMeta (that would mirror keyString and should return the meta value). Maybe we call it keyMetaData instead.

Since we will keep the existing metadata API, specifically keySetMeta and keyGetMeta, I agree that we should add a function that returns just the meta value of a metakey. I'm wondering if it's really useful if keyGetMeta returns the key and not the value itself? If possible, we should change that function.
Then keyMeta could be used to access the meta keyset. Otherwise keyMetaData is also a good option.

pass two times the same special pointer

Since this is only used internally and probably has only one occurence, I think it's fine.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

I'm wondering if it's really useful if keyGetMeta returns the key and not the value itself?

It was useful, but if the KeySet is accessible using ksLookup is just as good.

If possible, we should change that function.

Not sure that it is possible. It would break a huge amount of code.


pass two times the same special pointer addresses (which cannot happen on valid ksNew calls)? E.g. ksNew (2, 0xEADAEADA 0xEADAEADA, KS_END)

Why can't it happen? The code below is completely valid. It doesn't make much sense, but its not wrong. Or did you mean that the address 0xEADAEADA cannot happen?

Key * k = keyNew ("user/test", KEY_END);
KeySet * ks = ksNew (2, k, k, KS_END);

IMO we should just say ksNew (0) (KS_END is not needed with 0) doesn't allocate an array. Only when the first key is added, we allocate the array with the minimum size. This might also save allocations elsewhere, for example in something like this:

void foo (KeySet * ks)
{
  KeySet * result = ksNew (0);
  for (cursor_t i = 0; i < ksGetSize (ks); ++i)
  {
    Key * cur = ksAtCursor (ks, i);
    if (hasSomeProperty (cur))
    {
      ksAppendKey (result, someTransform (cur));
    }
  }

  doSomething (result);

  ksDel (result);
}
@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

Since we will keep the existing metadata API, specifically keySetMeta and keyGetMeta, I agree that we should add a function that returns just the meta value of a metakey. I'm wondering if it's really useful if keyGetMeta returns the key and not the value itself? If possible, we should change that function.

Yes, I agree! Originally, keyGetMeta needed to return a key to find out about identity of keys. This can after implementing the proposal also be done via ksLookupByName(keyMetaData(key), "meta/data/to/find")

So it would be fine if keyGetMeta returns a char*.

Then keyMeta could be used to access the meta keyset. Otherwise keyMetaData is also a good option.

I am for keyMeta, it is the shortest.

Since this is only used internally and probably has only one occurence, I think it's fine.

Actually it can be much simpler: We can make ksNew (0, KS_END) not make the array allocation in all cases. Then we can simply set the flag which makes the KeySet a meta-keyset.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2019

I think the key questions are answered here. I can work on this.

@PhilippGackstatter PhilippGackstatter self-assigned this Oct 14, 2019
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2019

Should keySetName(key, "namewithoutnamespace") still be allowed or not? And if so, what namespace would it have? I'd say it shouldn't be allowed. With the addition of meta/ for metakeys all valid keynames have a prefixed namespace (right?).

The background is this. The elektraNamespace enum already has a KEY_NS_META. It's definition is "metakey, i.e. any key name not under other categories". Since we want to change this to only apply to keys prefixed by meta/, we would need a new default category, if we wanted to still allow names without a defined namespace, since keyGetNamespace's default case is to return KEY_NS_META.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2019

Should keySetName(key, "namewithoutnamespace") still be allowed or not? And if so, what namespace would it have? I'd say it shouldn't be allowed.

I agree.

With the addition of meta/ for metakeys all valid keynames have a prefixed namespace (right?).

Yes. It depends on the effort, though. If it cannot be done within a few hours, we can also leave it as-is. I would say we make the meta:/ change together with the colon between namespaces (#1037). (There are both big changes and it is unknown if the changes are feasible.)

The background is this. The elektraNamespace enum already has a KEY_NS_META. It's definition is "metakey, i.e. any key name not under other categories". Since we want to change this to only apply to keys prefixed by meta/, we would need a new default category, if we wanted to still allow names without a defined namespace, since keyGetNamespace's default case is to return KEY_NS_META.

As the invalid key would not be possible, we do not need to have it in the enum.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2019

all valid keynames have a prefixed namespace

There are still the cascading keys that spec adds to the KeySet for their use as defaults. It might make sense to prefix these with default. Then keys without a namespace would only be used as a lookup and never inside a KeySet.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2019

There are still the cascading keys that spec adds to the KeySet for their use as defaults. It might make sense to prefix these with default. Then keys without a namespace would only be used as a lookup and never inside a KeySet.

Yes, good point! First I thought this is unrelated but actually if we would separate this we could disallow cascading keys in the key set. I created #3082 to describe the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.