Mount cli integration in C #4851

flo91 · 2023-02-28T10:55:05Z

Implements code for listing mountpoints and mounting new files (includes plugin-resolution and -handling for mounting) in C.
Most of the C++-based code is now implemented in C, but there is still some work left, including testing, and refactoring.

I'll write a posting in this PR and add the label needs review as soon as

I've successfully finished and
refactored the implementation,
tested some use cases and
reviewing seems feasible.

Thank you!

Basics

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which
contains _(my name)_)
Please always add something to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy. If not, fix in this order:
- add a line in doc/news/_preparation_next_release.md
- reformat the code with scripts/dev/reformat-all
- make all unit tests pass
- fix all memleaks
The PR is rebased with current master.

Checklist

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation (see Documentation Guidelines)
I fixed all affected decisions (see Decision Process)
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in reuse syntax

Review

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

early stage, lots of open TODOs, work in progress

work in progress, not compiling

@hannes99

from the new kdb command (work in progress!) based on parts of the branch "kdb_cli_rewrite" from @hannes99

and add new `kdb mountpoint list` command

and some refactoring

re-implement C++-based code for mounting in C

and include elektra-meta as target link lib for src/libs/elektra

needed for handling of plugins during mounting only draft code, still needs: doc, tests, refactoring

add code for checking conflicts of plugins

for KeySets for infos and symbols (allocation)

kodebach

I know this isn't done yet, but I had a quick look anyway.

It seems this is basically a 1:1 translation of the C++ code. That's a good start, but there are some things that need to be done differently now that we have backend plugins. I also noticed a few things that could be simplified.

kodebach · 2023-02-28T11:14:12Z

src/libs/elektra/cmount.c

+	}
+
+	/* STEP 2: Check for wrong namespace (proc) */
+	if (keyGetNamespace (mountPoint) == KEY_NS_PROC)


Mounting into proc:/ is in theory allowed by libelektra-kdb.

kodebach · 2023-02-28T11:19:38Z

src/libs/elektra/cmount.c

+
+	/* STEP 3: Check for name match */
+	const char * mpName = keyName (mountPoint);
+	if (keyGetNameSize (mountPoint) == 2 && *mpName == '/')


Seems rather complicated... Why not just do a cascading lookup and report the found key in the error?

Key * found = ksLookupByName (alreadyUsedMountpoints, mpName, 0); if (found != NULL) { fprintf (stderr, "Mountpoint '%s' not possible, because mountpoint '%s' already exists.\n", mpName, keyName (found)); }

This should work for all cases of mpName

kodebach · 2023-02-28T11:20:52Z

src/libs/elektra/cmount.c

+	ksDel (alreadyUsedMountpoints);
+	ksDel (backendInfos);
+
+	/* TODO: STEP 4: Check if mounted below system:/elektra */


Suggested change

/* TODO: STEP 4: Check if mounted below system:/elektra */

/* TODO: STEP 4: Check if mounted below /elektra in any namespace */

kodebach · 2023-02-28T11:22:04Z

src/libs/elektra/cmount.c

+	/* Remove namespace */
+	for (int i = 0; i < keyGetNameSize (elektraCheck); ++i)
+	{
+		if (*(mpName + i) == ':')
+		{
+			keySetName (elektraCheck, mpName + i + 1);
+		}
+	}


Pretty sure the keyIsBelow* functions handle cascading keys too. Otherwise, please use:

Suggested change

/* Remove namespace */

for (int i = 0; i < keyGetNameSize (elektraCheck); ++i)

{

if (*(mpName + i) == ':')

{

keySetName (elektraCheck, mpName + i + 1);

}

}

/* Remove namespace */

keySetNamespace (elektraCheck, KEY_NS_CASCADING);

kodebach · 2023-02-28T11:22:52Z

src/libs/elektra/cmount.c

+			size_t lenLookup = keyGetNameSize (cur); /* includes \0 */
+			if (lenLookup > 1)
+			{
+				lenLookup += strlen ("/definition/path");


That key is specific to the backend plugin. We need a more general solution...

kodebach · 2023-02-28T11:24:10Z

src/libs/elektra/cmount.c

+ *
+ * @return true if the provided str is a valid plugin-name, false otherwise
+ */
+bool validatePluginName (const char * str)


AFAIK the name of plugins isn't actually validated anywhere else. The mount code should accept anything that is allowed by libelektra-kdb and elektraPlugin*.

kodebach · 2023-02-28T11:32:51Z

src/tools/kdb/mountpoint-mount.c

+		elektraKeyToBoolean (tmp, &optForce);
+	if ((tmp = GET_OPTION_KEY (options, "quiet")))
+		elektraKeyToBoolean (tmp, &optQuiet);
+	if ((tmp = GET_OPTION_KEY (options, "interactive")))


AFAIK we planned to remove the interactive mode, since nobody is using it anyway

kodebach · 2023-02-28T11:35:05Z

src/tools/kdb/mountpoint-mount.c

+	if (!optQuiet && *argPath == '/' && keyMpNs != KEY_NS_SYSTEM && keyMpNs != KEY_NS_SPEC && keyMpNs != KEY_NS_CASCADING)
+	{
+		printf("Note that absolute paths are still relative to their namespace (see `kdb plugin-info resolver`).\n");
+		printf("Only system+spec mountpoints are actually absolute.\n");


That still depends on the backend plugin and for backend on resolver plugin. It's just that the default resolver does it this way

kodebach · 2023-02-28T11:35:26Z

src/tools/kdb/mountpoint-mount.c

+		printf("Note that absolute paths are still relative to their namespace (see `kdb plugin-info resolver`).\n");
+		printf("Only system+spec mountpoints are actually absolute.\n");
+		printf("Use `kdb file %s` to determine where the file(s) are.\n", argMountpoint);
+		printf("Use `-q` or use `kdb set %s/quiet 1` to suppress infos.\n", CLI_BASE_KEY);


Suggested change

printf("Use `-q` or use `kdb set %s/quiet 1` to suppress infos.\n", CLI_BASE_KEY);

printf("Use `-q` or use `kdb set %s/quiet 1` to suppress all infos like this.\n", CLI_BASE_KEY);

flo91 · 2023-03-06T16:49:13Z

As this PR gets merged into ElektraInitiative:mount_cli_integration and not into master,
I'll merge the current state of this PR as requested by @hannes99.
So he can rebase his branch with current master and has the current state of the mount-implementation.

After the rebasing is done, I'll open a new PR with the changes since this PR.

flo91 added 13 commits January 22, 2023 11:36

Mounting: Re-implement C++-code for mounting in C

5fd1c43

early stage, lots of open TODOs, work in progress

Mounting: Continue re-implemnting mounting-related C++-code in C

1d1878f

work in progress, not compiling

kdb-cli: add mount command and call the library

bc63b86

from the new kdb command (work in progress!) based on parts of the branch "kdb_cli_rewrite" from @hannes99

Merge branch 'c-mount' into mount_cli_integration

c3d0de6

and add new `kdb mountpoint list` command

mounting: refactor kdb-cli for usage with new lib

cc38478

mounting: Add mountpoint validation to c-based lib

9277a65

and some refactoring

mounting: Refactoring, start processing of plugins

3c660d7

mounting: Process PluginSpec for adding plugins

d36cd24

mounting: Implement lookupProvides() for plugins

e5e2bd4

re-implement C++-based code for mounting in C

mounting: Add code for adding and sorting plugins

5089d20

and include elektra-meta as target link lib for src/libs/elektra

mounting: resolveNeeds - Get missing recommends

de56f92

needed for handling of plugins during mounting only draft code, still needs: doc, tests, refactoring

mounting: Add lists for strings and plugin-specs

b7a53e1

add code for checking conflicts of plugins

mounting: add checking of config, fix args

107d530

for KeySets for infos and symbols (allocation)

flo91 added work in progress lang/c labels Feb 28, 2023

flo91 self-assigned this Feb 28, 2023

flo91 mentioned this pull request Feb 28, 2023

[DRAFT] Mounting in C #4626

Closed

23 tasks

kodebach reviewed Feb 28, 2023

View reviewed changes

atmaxinger self-requested a review February 28, 2023 12:15

kodebach mentioned this pull request Feb 28, 2023

Testframework for applications #4852

Closed

flo91 removed the work in progress label Mar 6, 2023

flo91 marked this pull request as ready for review March 6, 2023 16:49

flo91 merged commit 4fa9ca8 into ElektraInitiative:mount_cli_integration Mar 6, 2023

flo91 mentioned this pull request Mar 9, 2023

WIP: Mount tooling in C #4864

Closed

19 tasks

flo91 changed the title ~~WIP: Mount cli integration in C~~ Mount cli integration in C Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mount cli integration in C #4851

Mount cli integration in C #4851

flo91 commented Feb 28, 2023 •

edited

kodebach left a comment

kodebach Feb 28, 2023

kodebach Feb 28, 2023

kodebach Feb 28, 2023

kodebach Feb 28, 2023

kodebach Feb 28, 2023

kodebach Feb 28, 2023

kodebach Feb 28, 2023

kodebach Feb 28, 2023

kodebach Feb 28, 2023

flo91 commented Mar 6, 2023

	/* TODO: STEP 4: Check if mounted below system:/elektra */
	/* TODO: STEP 4: Check if mounted below /elektra in any namespace */

	printf("Use `-q` or use `kdb set %s/quiet 1` to suppress infos.\n", CLI_BASE_KEY);
	printf("Use `-q` or use `kdb set %s/quiet 1` to suppress all infos like this.\n", CLI_BASE_KEY);

Mount cli integration in C #4851

Mount cli integration in C #4851

Conversation

flo91 commented Feb 28, 2023 • edited

Basics

Checklist

Review

Labels

kodebach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flo91 commented Mar 6, 2023

flo91 commented Feb 28, 2023 •

edited