Unify socket datatypes #250

james-knippes · 2023-10-23T14:32:38Z

Hey, I've started to work on getting the datatypes used in the socket communication unified. (See #179 ) The types are now defined in control-datatypes.h .

The process led to a little bit of refactoring of the control commands in control-cmds.h .

Before I go on with the other datatypes and control commands I would appriciate some feedback if you have the time @mobin-2008 or @davmac314 .

davmac314 · 2023-10-23T22:26:53Z

Looks good to me so far, I don't have any concerns other than minor things I'll comment.

davmac314 · 2023-10-23T22:34:03Z

src/control.cc

            // remove required mark, stop if not required by dependents
            if (do_pin) service->pin_stop();
            service->stop(false);
            services->process_queues();
            if (service->get_state() == service_state_t::STOPPED) ack_buf[0] = DINIT_RP_ALREADYSS;
            break;
+        default:
+            // can't get here (hopefully)
+            // supreses compiler warning about unused  (-Wswitch)


Please keep the comment to a minimum - since this is just for suppressing a warning, as little noise as possible is preferable. I suggest:

// avoid warning for unhandled values in switch/case

davmac314 · 2023-10-23T22:35:35Z

src/includes/control-datatypes.h

+
+#include <cstdint>
+
+namespace dinit_ctypes {


ctypes sounds a bit too close to the existing character types (std::ctype), maybe dinit_cptypes or dinit_cp_types (for "command protocol types")?

seems reasonable to avoid confusion. it's dinit_cptypes now

davmac314 · 2023-10-23T22:37:32Z

Just the two comments, otherwise looks good, thanks!

james-knippes · 2023-10-25T19:57:50Z

Thanks for the Feedback @davmac314 . I've continued refactoring in the other datatypes used in communication in the same way. I think by now i covered most of them.

To be honest, I have some difficulties reading/understanding the protocol related code. It feels a bit 'fragile' with all the hardcoded values/lengths in different places. Most of it should be avoidable with some rework using sizeof/types. Or we could move the static parts of the protocol into standard layout type structs?

davmac314 · 2023-10-25T22:27:32Z

To be honest, I have some difficulties reading/understanding the protocol related code. It feels a bit 'fragile' with all the hardcoded values/lengths in different places. Most of it should be avoidable with some rework using sizeof/types. Or we could move the static parts of the protocol into standard layout type structs?

It is slightly tricky code. A lot of it already uses sizeof where it can (except for when it would be sizeof(char), it will just use 1). I certainly am happy if you change code which is using fixed sizes > 1 to use a sizeof instead.

Using regular structs is a bit less simple, since you'd need to make sure they were exactly compatible with the current protocol. Standard C++ has no way to specify a packed struct as far as I know, so this would be only be possible where the packet format already matches standard layout. If you could keep within those constraints I'm happy for such changes, but I think that should be part of a second change set (not part of this PR).

davmac314 · 2023-10-25T22:31:52Z

src/includes/control-datatypes.h

+    using cp_info_t = uint8_t;
+    using srvname_len_t = uint16_t;
+    using envvar_len_t = uint16_t;
+    using sig_num_t = int32_t;


This one (sig_num_t) isn't correct, a signal number is an int and not guaranteed to be 32-bit.

Yes, I'm aware that it is not guaranteed to be 32-bit.
The question here would be do we want the protocol to be stable (even across architectures/systems) so it could be used over system boundaries (e.g. over network and/or forwarded to container/vm) ? In that case I would propose to leave it be fixed to 32-bit for the protocol and cast to int in the application. When there is a system, where int is not 32-bit then we may need to handle the conversion in dinitctl differently with an additional check, warning or error.

I've never had the goal of having the protocol be usable over system boundaries with heterogeneous systems. That requires more than just consistently sized values; you need to ensure consistent byte order, as well, and no struct declaration will do that for you.

I know that for the most part int32_t will work just fine - 99% of the time it will be the case that sizeof(int) == sizeof(int32_t) - and the remainder of the time signal numbers will probably not be bigger than 2^31 anyway. However, although it's a bit arbitrary, "works on any POSIX system" is a design goal and POSIX just says that signal numbers are int.

So we should be in the clear here, as dinitctl handles the out_of_range exception if the specified signal number does not fit into a sig_num_t/int32_t. I've also added a cast from int32_t to int in control.cc, to make it more explicit. So when there is a system with different sizes it should not break, we just don't support signal numberrs >32bit atm.

I may have been unclear. Sorry, but no - the signal number type needs to be int. That's what POSIX declares it as.

as dinitctl handles the out_of_range exception if the specified signal number does not fit into a sig_num_t/int32_t

dinitctl uses std::stoi to convert the signal number and that returns int. It only throws out_of_range if the number doesn't fit into an int - it doesn't know anything about sig_num_t/int32_t. So if int is larger than 32-bits there's no checking done in dinitctl that the provided signal number will fit in int32_t. Just assigning the result to int32_t will silently truncate it. (Or am I missing something?)

we just don't support signal numberrs >32bit atm

That would be a regression introduced with this change; currently, we do in fact support signal numbers >32 bit.

Everything else in the PR looks good!

Ah.. now I get it. I've reverted sig_num_t to be int. And I've checked again, that at least in the signal protocol code sizeof is used consequently, so it doesn't break the protocol on systems where sizeof(int) != 4.

davmac314 · 2023-10-25T22:33:45Z

src/control.cc

-        // DINIT_RP_CVERSION, (2 byte) minimum compatible version, (2 byte) actual version
-        char replyBuf[] = { DINIT_RP_CPVERSION, 0, 0, 0, 0 };
+        // cp_rply::CVERSION, (2 byte) minimum compatible version, (2 byte) actual version
+        char replyBuf[] = { (cp_rply_t)cp_rply::CPVERSION, 0, 0, 0, 0 };


Casts should be to the destination type, which would be char here, if needed at all.

(cp_rply_t)cp_rply::CPVERSION is superfluous - the type of cp_rply::CPVERSION is already cp_reply_t.

I think a cast is actually necessary here, because cp_rply::CPVERSION is of type cp_rply and not cp_rply_t, as it's an enum class.

I've looked it up and narrowing is also applied on static variables in initializer lists. The difference between casting to char or cp_rply_t type in this case would mostly be, that the compiler complains as soon as the value would be out of range for the target type (char, so >127 in this case). So I agree casting to (char) is probably best here.

Right, you're correct, I misread it - still, casting to char is probably best, as you say.

I've changed all relevant casts to (char).

davmac314 · 2023-10-25T22:34:29Z

src/control.cc

@@ -76,7 +81,7 @@ bool control_conn_t::process_packet()
            auto sd_type = static_cast<shutdown_type_t>(rbuf[1]);

            services->stop_all_services(sd_type);
-            char ackBuf[] = { DINIT_RP_ACK };
+            char ackBuf[] = { (cp_rply_t)cp_rply::ACK };


Similarly unnecessary/incorrect cast here, and a bunch throughout, I think.

davmac314 · 2023-10-25T22:39:59Z

src/control.cc

    }

    if (record == nullptr) {
-        std::vector<char> rp_buf = { fail_code };
+        std::vector<char> rp_buf = { (cp_rply_t)fail_code };


(This one is causing build failure on Mac, clang is less lenient than gcc)

The underlying Problem seems to be, that when variables in the initilizer list are non static and the types are not the same then there is narrowing involved (which I don't fully understand). So casting to (char) works, but casting to (cp_rply_t)/(unsigned char) does not, as it's not clear if the value fits into char. So I've opted for casting to (char) in this case.

davmac314 · 2023-10-25T22:41:15Z

src/includes/control-cmds.h

 // Dinit control command packet types

 // Requests:
+enum class cp_cmd :dinit_cptypes::cp_cmd_t {


formatting nit: can you put spaces around : to match style elsewhere? i.e

enum class cp_cmd : dinit_cptypes::cp_cmd_t {

davmac314 · 2023-10-25T22:42:04Z

Had a quick look through and added some comments, mostly looking good though, thanks!

james-knippes · 2023-10-27T08:06:45Z

It is slightly tricky code. A lot of it already uses sizeof where it can (except for when it would be sizeof(char), it will just use 1). I certainly am happy if you change code which is using fixed sizes > 1 to use a sizeof instead.

Yes i will create an new issue/PR for that to no clutter this PR to much.

Using regular structs is a bit less simple, since you'd need to make sure they were exactly compatible with the current protocol. Standard C++ has no way to specify a packed struct as far as I know, so this would be only be possible where the packet format already matches standard layout. If you could keep within those constraints I'm happy for such changes, but I think that should be part of a second change set (not part of this PR).

Ah I didn't thought about packed structs not being standard c++. As we are currently only targeting compilers, which both (gcc and clang) support attribute((packed)) for structs we may use it? Could discuss this in a new Issue if you don't rule it out. Maybe also discuss other possible ways to optimize the protocol (handling).

james-knippes · 2023-10-27T10:00:28Z

The remaining issues should be fixed now. Let me know if i missed something. I will squash the fixes into the related commits later.

davmac314 · 2023-10-27T10:07:55Z

As we are currently only targeting compilers, which both (gcc and clang) support __attribute((packed))__ for structs

I don't want to limit support to just those compilers, but that's not necessarily a deal-breaker for using structs with attribute((packed)), if it is available (as long as it's behind a guard so it's still possible to compile with other compilers, with the caveat that the struct layout will be different in that case).

In hindsight I should've just used standard layout structs from the beginning and then we wouldn't need ((packed)). There's not a lot of point in saving a few bytes in the protocol messages. But, too late for that now.

mobin-2008 · 2023-10-28T10:05:44Z

src/includes/control.h

@@ -15,6 +15,7 @@
 #include <control-cmds.h>
 #include <service-listener.h>
 #include <cpbuffer.h>
+#include "control-datatypes.h"


Hi, I reviewd your work and it looks good, unless that control-datatypes.h should included like this, I guess (with < > not " "):

#include <control-datatypes.h>

Hey, nice catch thanks. Functionally it should not make a difference in this case. But I've changed the includes of control-datatypes.h so they are consistent with the others in the corresponding file.

…class - also rename cmd_t and command_t (to cp_cmd_t and ctl_cmd) to reduce ambiguity

…essages as enum class

james-knippes · 2023-10-30T13:31:16Z

After fixing the signal type misunderstanding on my part and the includes, I squashed and rebased everything. It should be ready to merge after the pipeline checks are good.

mobin-2008

Looks good, Thanks for contribution 😁

davmac314 · 2023-10-30T21:18:14Z

Thanks! Merged!

mobin-2008 added Enhancement/New Feature Improving things or introduce new feature A-Importance: Normal C-control Things about custom control protocol of Dinit labels Oct 23, 2023

davmac314 reviewed Oct 23, 2023

View reviewed changes

james-knippes force-pushed the feature_unify-socket-datatypes branch from 33cf6ed to 4109481 Compare October 25, 2023 19:39

davmac314 reviewed Oct 25, 2023

View reviewed changes

james-knippes marked this pull request as ready for review October 27, 2023 10:00

mobin-2008 reviewed Oct 28, 2023

View reviewed changes

james-knippes added 9 commits October 30, 2023 14:23

control-datatypes.h: unifty handle_t

4bc067f

control-datatypes.h: add trigger_val_t

7b2fb53

control-datatypes.h: add cmd_t and refactor control commands to enum …

4d357f8

…class - also rename cmd_t and command_t (to cp_cmd_t and ctl_cmd) to reduce ambiguity

control-datatypes: refactor control protocl replies and information m…

bb4e124

…essages as enum class

control-datatypes: add srvname_len_t

a2cfe5f

control-datatypes: add envvar_len_t

b45a043

control-datatypes: add sig_num_t

af3bfbc

control-datatypes: add srvstate_t

0fe7ddb

apply standard formatting for enum base type

33a2bc4

james-knippes force-pushed the feature_unify-socket-datatypes branch from 0ea8adc to 33a2bc4 Compare October 30, 2023 13:26

mobin-2008 approved these changes Oct 30, 2023

View reviewed changes

mobin-2008 requested a review from davmac314 October 30, 2023 15:40

davmac314 merged commit 2223b74 into davmac314:master Oct 30, 2023
11 checks passed

mobin-2008 mentioned this pull request Nov 1, 2023

unifying the data types for socket communication #179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify socket datatypes #250

Unify socket datatypes #250

james-knippes commented Oct 23, 2023

davmac314 commented Oct 23, 2023

davmac314 Oct 23, 2023

james-knippes Oct 25, 2023

davmac314 Oct 23, 2023 •

edited

Loading

james-knippes Oct 25, 2023

davmac314 commented Oct 23, 2023

james-knippes commented Oct 25, 2023

davmac314 commented Oct 25, 2023

davmac314 Oct 25, 2023

james-knippes Oct 27, 2023

davmac314 Oct 27, 2023

james-knippes Oct 27, 2023

davmac314 Oct 28, 2023

james-knippes Oct 30, 2023

davmac314 Oct 25, 2023

james-knippes Oct 27, 2023

davmac314 Oct 27, 2023

james-knippes Oct 27, 2023

davmac314 Oct 25, 2023

davmac314 Oct 25, 2023

james-knippes Oct 27, 2023

davmac314 Oct 25, 2023

james-knippes Oct 27, 2023

davmac314 commented Oct 25, 2023

james-knippes commented Oct 27, 2023 •

edited

Loading

james-knippes commented Oct 27, 2023

davmac314 commented Oct 27, 2023

mobin-2008 Oct 28, 2023

james-knippes Oct 30, 2023

james-knippes commented Oct 30, 2023

mobin-2008 left a comment

davmac314 commented Oct 30, 2023

Unify socket datatypes #250

Unify socket datatypes #250

Conversation

james-knippes commented Oct 23, 2023

davmac314 commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davmac314 Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davmac314 commented Oct 23, 2023

james-knippes commented Oct 25, 2023

davmac314 commented Oct 25, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davmac314 commented Oct 25, 2023

james-knippes commented Oct 27, 2023 • edited Loading

james-knippes commented Oct 27, 2023

davmac314 commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-knippes commented Oct 30, 2023

mobin-2008 left a comment

Choose a reason for hiding this comment

davmac314 commented Oct 30, 2023

davmac314 Oct 23, 2023 •

edited

Loading

james-knippes commented Oct 27, 2023 •

edited

Loading