[k2] add file functions in runtime light #1161

astrophysik · 2024-11-20T16:35:14Z

todo

Support php globals STDERR, STDOUT, STDIN
move constant INVALID_PLATFORM_DESCRIPTOR to k2-api.h

apolyakov

General comment: please apply clang-format to runtime-light and runtime-common directories (files you modified).

runtime-common/stdlib/stream/stream-context.h

runtime-common/stdlib/stream/stream-functions.h

runtime-light/stdlib/resource/resource-state.cpp

runtime-light/stdlib/stdlib.cmake

apolyakov · 2024-12-08T10:34:47Z

runtime-light/k2-platform/k2-api.h

@@ -165,4 +165,26 @@ inline auto arg_fetch(uint32_t arg_num) noexcept {
                        std::unique_ptr<char, decltype(std::addressof(k2::free))>{value_buffer, std::addressof(k2::free)});
 }

+inline int32_t udp_connect(uint64_t *socket_d, const char *host, size_t host_len) {


Missing noexcept modifier. Please, apply it to all the functions you' added

apolyakov · 2024-12-08T10:38:07Z

runtime-light/k2-platform/k2-api.h

+}
+
+
+inline int32_t lookup_host(const char *host, size_t host_len, struct SockAddr *result_buf, size_t *result_buf_len) {


We should not depend on raw k2 primitives in our code. Please, use a simple using SockAddr = SockAddr;. It should be applied to all k2 types you use.

apolyakov · 2024-12-08T10:38:46Z

runtime-light/k2-platform/k2-api.h

+  return k2_lookup_host(host, host_len, result_buf, result_buf_len);
+}
+
+inline int32_t socket(uint64_t *socket_d, int32_t domain, int32_t type, int protocol) {


Please, don't use int. Use int32_t instead.

apolyakov · 2024-12-08T10:40:56Z

runtime-light/state/instance-state.h

@@ -100,6 +100,8 @@ struct InstanceState final : vk::not_copyable {
    return open_stream(std::string_view{component_name.c_str(), static_cast<size_t>(component_name.size())});
  }

+  std::pair<uint64_t, int32_t> connect_to(const string &host, bool reliable) noexcept;


I don't think that it's necessary to return low-level cerrno-like objects from this function

Also, I don't like the naming here: reliable?

apolyakov · 2024-12-08T10:41:31Z

runtime-light/state/instance-state.cpp

+  const int32_t error_number{reliable ? k2::tcp_connect(std::addressof(stream_d), host.c_str(), host.size())
+                                      : k2::udp_connect(std::addressof(stream_d), host.c_str(), host.size())};
+
+  if (error_number != 0) {


There is a special k2::errno_ok value for that purpose

apolyakov · 2024-12-08T10:57:11Z

runtime-light/stdlib/file/file-stream-functions.cpp

+    return false;
+  }
+
+  return {string(static_cast<int64_t>(type)).append(":").append(static_cast<int64_t>(stream_d))};


Can you please explain the idea behind these streams? As far as I understand, the user now has access to the internals of the runtime, e.g. internal stream descriptors.

apolyakov · 2024-12-08T11:00:30Z

runtime-light/stdlib/file/file-stream-functions.cpp

+    php_warning("try to fclose wrong resource %s", stream.to_string().c_str());
+    return false;
+  }
+  if (InstanceState::get().standard_stream() == stream_d) {


That is consecutive of your decision to operate on internal stream descriptors

apolyakov · 2024-12-08T11:01:48Z

runtime-light/stdlib/file/php-streams/php-streams.cpp

I don't see any reason to create a distinct directory for that

apolyakov · 2024-12-08T11:03:11Z

runtime-light/stdlib/file/php-streams/php-streams.cpp

+
+namespace {
+
+constexpr std::string_view STDERR_NAME{"stderr"};


Let's initialise globals with =. It's just how we started doing it... Also, as I can see, you also use it in another file.

apolyakov · 2024-12-08T11:05:17Z

runtime-light/stdlib/file/php-streams/php-streams.cpp

+} // namespace
+
+uint64_t open_php_stream(const string &stream_name) noexcept {
+  if (strcmp(stream_name.c_str(), STDERR_NAME.data()) == 0) {


You do have std::string_view here, why not just create another one and compare string views instead of raw pointers?

if (std::string_view{stream_name.c_str(), stream_name.size()} == STDERR_NAME)

astrophysik added runtime Feature related to runtime k2 k2 related labels Nov 20, 2024

astrophysik self-assigned this Nov 20, 2024

astrophysik force-pushed the vsadokhov/add-file-builtins branch from ec11160 to 27888ed Compare November 20, 2024 16:37

astrophysik marked this pull request as ready for review November 20, 2024 16:40

astrophysik requested a review from apolyakov November 20, 2024 16:53

astrophysik added this to the next milestone Nov 21, 2024

apolyakov requested changes Nov 21, 2024

View reviewed changes

apolyakov previously approved these changes Nov 22, 2024

View reviewed changes

astrophysik marked this pull request as draft November 22, 2024 11:38

astrophysik dismissed apolyakov’s stale review via 95cdd8a December 2, 2024 08:56

astrophysik force-pushed the vsadokhov/add-file-builtins branch from 5c2db15 to 95cdd8a Compare December 2, 2024 08:56

astrophysik changed the title ~~move stream functions to runtime common~~ add file functions in runtime light Dec 2, 2024

astrophysik marked this pull request as ready for review December 4, 2024 15:12

astrophysik requested a review from apolyakov December 4, 2024 15:12

astrophysik marked this pull request as draft December 4, 2024 15:22

astrophysik force-pushed the vsadokhov/add-file-builtins branch from e9b2935 to b3150bb Compare December 6, 2024 13:11

astrophysik marked this pull request as ready for review December 6, 2024 13:47

apolyakov requested changes Dec 8, 2024

View reviewed changes

astrophysik changed the title ~~add file functions in runtime light~~ [k2] add file functions in runtime light Dec 9, 2024

astrophysik removed this from the next milestone Dec 9, 2024

astrophysik added 9 commits December 12, 2024 15:51

add file builtins

dfb8797

add doc comment

571ef21

small fix

0ba5dac

add doc comment

a4f657f

remove less include

20a762d

small fix

93954a3

small fix

da70064

global rework

322848a

add virtual destructor

c78bf96

astrophysik added 15 commits December 12, 2024 15:51

small fix

e3e875d

small fix

6c09e02

small fix

8e46779

small fix

a90c531

small fix

4626df1

small fix

bab0bf3

remove connect_to

5fab2e8

remove php streams context

f429838

correct scheme checking

2b0d2ed

rename open udp connection

a6da74f

remove wrappers

cfb785b

add stream invalidation

4933520

add stdin, stdout, stderr

c91f2af

add stdin, stdout, stderr

53e5f5d

fix file_get_contents

97cc8aa

astrophysik force-pushed the vsadokhov/add-file-builtins branch from d2b3e5c to 97cc8aa Compare December 12, 2024 13:02

apolyakov force-pushed the vsadokhov/add-file-builtins branch from e612f28 to 5690961 Compare December 12, 2024 16:35

apolyakov added 3 commits December 13, 2024 00:33

A bit of refactoring

637d355

Move INVALID_PLATFORM_DESCRIPTOR inside k2 namespace

d31530c

Check more conditions before opening a stream

61e42a7

apolyakov force-pushed the vsadokhov/add-file-builtins branch from 5690961 to 61e42a7 Compare December 12, 2024 22:19

apolyakov self-requested a review December 12, 2024 22:19

apolyakov approved these changes Dec 13, 2024

View reviewed changes

apolyakov merged commit 28c7b9e into master Dec 13, 2024
5 checks passed

apolyakov deleted the vsadokhov/add-file-builtins branch December 13, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[k2] add file functions in runtime light #1161

[k2] add file functions in runtime light #1161

astrophysik commented Nov 20, 2024 •

edited

Loading

apolyakov left a comment

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

apolyakov Dec 8, 2024

		}


		inline int32_t lookup_host(const char host, size_t host_len, struct SockAddr result_buf, size_t *result_buf_len) {


		namespace {

		constexpr std::string_view STDERR_NAME{"stderr"};

[k2] add file functions in runtime light #1161

[k2] add file functions in runtime light #1161

Conversation

astrophysik commented Nov 20, 2024 • edited Loading

apolyakov 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

astrophysik commented Nov 20, 2024 •

edited

Loading