CTL: Add a CTL sources to the UMF #913

KFilipek · 2024-11-19T13:03:46Z

Description

This commit introduces sources and compilation of the CTL.

Checklists

TODO:

Move CTL sources
Make code compiled and symbols are exposed
Run tests

Other:

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

CI workflows, not executed per PR (e.g. Nightly), execute properly
New tests added, especially if they will fail without my changes
Added/extended example(s) to cover this functionality
Extended the README/documentation
All newly added source files have a license
All newly added source files are referenced in CMake files
Logger (with debug/info/... messages) is used

src/ctl/alloc.h

src/ctl/os.h

src/ctl/queue.h

src/ctl/ctl.h

src/CMakeLists.txt

src/ctl/ctl.c

lukaszstolarczuk · 2024-11-19T13:49:24Z

src/ctl/ctl.h

+ * ctl.h -- internal declaration of statistics and control related structures
+ */
+
+#ifndef PMDK_CTL_H


pls update all define's to use naming like: UMF_*_H

not done, e.g. CTL_DEBUG_H

Look once again, this snippet is outdated

src/ctl/ctl.h

src/CMakeLists.txt

src/ctl/ctl.c

lukaszstolarczuk · 2024-11-19T13:56:09Z

src/ctl/ctl.c

+ */
+int ctl_load_config_from_string(struct ctl *ctl, void *ctx,
+                                const char *cfg_string) {
+    // LOG(3, "ctl %p ctx %p cfg_string \"%s\"", ctl, ctx, cfg_string);


LOG could be replaced with our LOG_* macros

Temporarily deleted

I'll leave this issue open, for LOGs to come back, when ready

src/ctl/ctl.h

pbalcer · 2024-11-22T10:31:46Z

src/ctl/alloc.c

+    return fn_malloc(size);
+}
+
+static __thread int realloc_num;


do we need this fault injection? this was part of testing infra.

Removed, thanks

src/ctl/ctl.c

src/ctl/ctl_debug.c

src/libumf.def

test/CMakeLists.txt

test/ctl/test.cpp

bratpiorka · 2024-12-12T08:15:07Z

test/ctl/test.cpp

+    int value = 0;
+    ctl_query(ctl_handler, NULL, CTL_QUERY_PROGRAMMATIC,
+              "debug.heap.alloc_pattern", CTL_QUERY_READ, &value);
+    ASSERT_EQ(value, 1);


we definitely need more complex tests

bratpiorka · 2024-12-12T08:15:40Z

@KFilipek please consider moving this PR to draft and re-request review when ready

lukaszstolarczuk

now all changes are in a single commit, at best the original code should be in the first commit and all our changes in a sep. one - this would be easier to distingush our changes, e.g. in case of any issues found

lukaszstolarczuk · 2024-12-12T09:28:37Z

cmake/helpers.cmake

@@ -232,9 +232,8 @@ function(add_umf_target_compile_options name)
            PRIVATE -fPIC
                    -Wall
                    -Wextra
-                    -Wpedantic


I you want to add this change in this PR, it should be in a sep. commit

lukaszstolarczuk · 2024-12-12T09:31:03Z

src/ctl/ctl.h

+ * ctl.h -- internal declaration of statistics and control related structures
+ */
+
+#ifndef PMDK_CTL_H


not done, e.g. CTL_DEBUG_H

lukaszstolarczuk · 2024-12-12T09:33:08Z

src/ctl/ctl.c

@@ -0,0 +1,590 @@
+// SPDX-License-Identifier: BSD-3-Clause


it's usually first copyright, then a license txt, then SPDX tag, e.g.

/* * * Copyright (C) 2023-2024 Intel Corporation * * Under the Apache License v2.0 with LLVM Exceptions. See LICENSE.TXT. * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception * */

? 😉

I cannot reply in thread related with that you see
#913 (comment)

This commit introduces sources and compilation of the CTL. The CTL sources are copied from: https://github.com/pmem/pmdk/tree/master/src/common Signed-off-by: Krzysztof Filipek <[email protected]>

PatKamin · 2024-12-13T15:21:04Z

cmake/helpers.cmake

                    -Wformat-security
-                    -Wcast-qual
+                    -Wno-cast-qual


Why do you disable this flag?

PatKamin · 2024-12-13T21:50:34Z

src/ctl/ctl.h

+};
+
+struct ctl_argument {
+    size_t dest_size;                     /* sizeof the entire argument */


nit: size of

PatKamin · 2024-12-13T22:13:12Z

src/ctl/ctl.c

+ * This function opens up the config file, allocates a buffer of size equal to
+ * the size of the file, reads its content and sanitizes it for ctl_load_config.
+ */
+#ifdef WINDOWS_API_NEEDED


Unknown define. The code looks like it should work on linux.

PatKamin · 2024-12-13T22:17:12Z

src/ctl/ctl.c

+    if (endptr == str || errno != 0) {
+        return LLONG_MIN;
+    }
+    errno = olderrno;


Does errno have to be restored?

PatKamin · 2024-12-13T22:27:33Z

src/ctl/ctl.c

+	 * query has been handled.
+	 */
+    struct ctl_index_utlist *indexes = NULL;
+    indexes = Zalloc(sizeof(*indexes));


Check for NULL

KFilipek self-assigned this Nov 19, 2024

KFilipek added the enhancement New feature or request label Nov 19, 2024

lplewa requested changes Nov 19, 2024

View reviewed changes

src/ctl/alloc.h Outdated Show resolved Hide resolved

src/ctl/os.h Outdated Show resolved Hide resolved

src/ctl/queue.h Outdated Show resolved Hide resolved

lukaszstolarczuk reviewed Nov 19, 2024

View reviewed changes

KFilipek force-pushed the ctl-base branch 8 times, most recently from 5c52d3a to cdd3d46 Compare November 22, 2024 10:30

pbalcer reviewed Nov 22, 2024

View reviewed changes

KFilipek force-pushed the ctl-base branch 15 times, most recently from 4f30764 to 0f8383e Compare November 29, 2024 08:32

KFilipek force-pushed the ctl-base branch 2 times, most recently from 0d68c68 to 44ec630 Compare December 3, 2024 10:12

KFilipek force-pushed the ctl-base branch 8 times, most recently from dc1d821 to 76b70bc Compare December 11, 2024 13:19

KFilipek marked this pull request as ready for review December 11, 2024 13:22

KFilipek requested a review from a team as a code owner December 11, 2024 13:22

KFilipek force-pushed the ctl-base branch 4 times, most recently from b6d922a to 6cc7810 Compare December 11, 2024 17:16

bratpiorka requested changes Dec 12, 2024

View reviewed changes

lukaszstolarczuk reviewed Dec 12, 2024

View reviewed changes

KFilipek marked this pull request as draft December 12, 2024 10:03

KFilipek force-pushed the ctl-base branch 4 times, most recently from df3c47f to ecf3785 Compare December 12, 2024 16:24

Move PMDK CTL sources

888ab64

KFilipek force-pushed the ctl-base branch from ecf3785 to f663af4 Compare December 12, 2024 16:47

KFilipek added 2 commits December 12, 2024 17:53

[CMake] Disable pedantic mode and disable cast qualifier warning

ab2a19c

CTL: Add a CTL functionality to the UMF

05db80c

This commit introduces sources and compilation of the CTL. The CTL sources are copied from: https://github.com/pmem/pmdk/tree/master/src/common Signed-off-by: Krzysztof Filipek <[email protected]>

KFilipek force-pushed the ctl-base branch from f663af4 to 05db80c Compare December 12, 2024 16:53

bratpiorka marked this pull request as ready for review December 13, 2024 10:36

PatKamin reviewed Dec 13, 2024

View reviewed changes

cmake/helpers.cmake

-Wformat-security

-Wcast-qual

-Wno-cast-qual

Copy link

Contributor

PatKamin Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you disable this flag?

PatKamin reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTL: Add a CTL sources to the UMF #913

CTL: Add a CTL sources to the UMF #913

KFilipek commented Nov 19, 2024 •

edited

Loading

lukaszstolarczuk Nov 19, 2024

KFilipek Dec 6, 2024

lukaszstolarczuk Dec 12, 2024

KFilipek Dec 12, 2024

lukaszstolarczuk Nov 19, 2024

KFilipek Nov 22, 2024

lukaszstolarczuk Nov 22, 2024

pbalcer Nov 22, 2024

KFilipek Dec 6, 2024

bratpiorka Dec 12, 2024

bratpiorka commented Dec 12, 2024

lukaszstolarczuk left a comment

lukaszstolarczuk Dec 12, 2024

KFilipek Dec 12, 2024

lukaszstolarczuk Dec 12, 2024

lukaszstolarczuk Dec 12, 2024

KFilipek Dec 12, 2024

lukaszstolarczuk Dec 12, 2024

KFilipek Dec 12, 2024

PatKamin Dec 13, 2024

PatKamin Dec 13, 2024

PatKamin Dec 13, 2024

PatKamin Dec 13, 2024

PatKamin Dec 13, 2024

CTL: Add a CTL sources to the UMF #913

Are you sure you want to change the base?

CTL: Add a CTL sources to the UMF #913

Conversation

KFilipek commented Nov 19, 2024 • edited Loading

Description

Checklists

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

bratpiorka commented Dec 12, 2024

lukaszstolarczuk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KFilipek commented Nov 19, 2024 •

edited

Loading