update examples#13
Conversation
wokron
force-pushed
the
update-examples
branch
2 times, most recently
from
e9ffd00 to
e0f2a3b
Compare
wokron
force-pushed
the
update-examples
branch
from
e0f2a3b to
ab794d2
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates example programs in the repository by replacing the fast-cp example with a new link-cp example and refactoring the echo-server example to use direct file descriptors and fixed buffers.
Changes:
- Replaced
fast-cp.cppwithlink-cp.cppfeaturing command-line argument parsing for configurable task count and chunk size - Refactored
echo-server.cppto useasync_accept_direct,async_recv, andasync_sendwith fixed file descriptor tables - Updated
CMakeLists.txtto buildlink-cpinstead offast-cp
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| examples/CMakeLists.txt | Updated build target from fast-cp to link-cp |
| examples/fast-cp.cpp | Removed the old fast-cp implementation |
| examples/link-cp.cpp | Added new file copy example with command-line options for task configuration |
| examples/echo-server.cpp | Refactored to use direct file descriptors and moved socket setup to main function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (infd < 0 || outfd < 0) { | ||
| std::fprintf(stderr, "Failed to open file: %d %d\n", infd, outfd); | ||
| exit(1); | ||
| } | ||
|
|
||
| struct statx statx_buf; | ||
| int r_stat = co_await condy::async_statx( | ||
| AT_FDCWD, infile, AT_STATX_SYNC_AS_STAT, STATX_SIZE, &statx_buf); | ||
| if (r_stat < 0) { | ||
| std::fprintf(stderr, "Failed to statx file: %d\n", r_stat); | ||
| exit(1); | ||
| } | ||
|
|
||
| #if !IO_URING_CHECK_VERSION(2, 6) // >= 2.6 | ||
| int r_trunc = co_await condy::async_ftruncate( | ||
| outfd, static_cast<loff_t>(statx_buf.stx_size)); | ||
| if (r_trunc < 0) { | ||
| std::fprintf(stderr, "Failed to truncate file: %d\n", r_trunc); | ||
| exit(1); | ||
| } | ||
| #else | ||
| int r_trunc = ftruncate64(outfd, static_cast<loff_t>(statx_buf.stx_size)); | ||
| if (r_trunc < 0) { | ||
| std::fprintf(stderr, "Failed to truncate file: %d\n", errno); | ||
| exit(1); | ||
| } | ||
| #endif | ||
|
|
||
| std::printf("Copy %lld bytes from %s to %s\n", | ||
| (long long)statx_buf.stx_size, infile, outfile); | ||
|
|
||
| auto start = std::chrono::high_resolution_clock::now(); | ||
|
|
||
| co_await do_file_copy(infd, outfd, static_cast<loff_t>(statx_buf.stx_size)); | ||
|
|
||
| auto end = std::chrono::high_resolution_clock::now(); | ||
| std::chrono::duration<double> elapsed = end - start; | ||
| double mbps = | ||
| (static_cast<double>(statx_buf.stx_size) / (1024.0 * 1024.0)) / | ||
| elapsed.count(); | ||
| std::printf("Copied %lld bytes in %.2f seconds (%.2f MB/s)\n", | ||
| (long long)statx_buf.stx_size, elapsed.count(), mbps); | ||
|
|
||
| co_await (condy::async_close(infd) && condy::async_close(outfd)); |
There was a problem hiding this comment.
If the file size is not zero, but all error paths in co_main call exit(1) before reaching line 140, the file descriptors will not be properly closed. While exit() will eventually close them, it's better practice to ensure proper cleanup even in error paths. Consider using RAII or ensuring the close operations happen even on error paths.
| auto buf = condy::buffer(buffer, to_copy); | ||
|
|
||
| auto aw1 = condy::async_read( | ||
| condy::fixed(0), condy::fixed(buffer_index, buf), current_offset); | ||
| auto aw2 = condy::async_write( | ||
| condy::fixed(1), condy::fixed(buffer_index, buf), current_offset); | ||
| auto [r1, r2] = co_await (std::move(aw1) >> std::move(aw2)); | ||
|
|
||
| if (r1 < 0 || r2 < 0) { | ||
| std::fprintf(stderr, "Failed to copy at offset %lld: %d %d\n", | ||
| (long long)current_offset, r1, r2); | ||
| exit(1); |
There was a problem hiding this comment.
The code doesn't handle partial reads or writes. If async_read or async_write return fewer bytes than requested (which is valid behavior), the copied file may be incomplete or corrupted. The result values r1 and r2 should be checked to ensure they match the expected to_copy size, and the operations should be retried if necessary.
| auto buf = condy::buffer(buffer, to_copy); | |
| auto aw1 = condy::async_read( | |
| condy::fixed(0), condy::fixed(buffer_index, buf), current_offset); | |
| auto aw2 = condy::async_write( | |
| condy::fixed(1), condy::fixed(buffer_index, buf), current_offset); | |
| auto [r1, r2] = co_await (std::move(aw1) >> std::move(aw2)); | |
| if (r1 < 0 || r2 < 0) { | |
| std::fprintf(stderr, "Failed to copy at offset %lld: %d %d\n", | |
| (long long)current_offset, r1, r2); | |
| exit(1); | |
| loff_t remaining = to_copy; | |
| loff_t chunk_offset = current_offset; | |
| auto base_ptr = static_cast<char *>(buffer); | |
| while (remaining > 0) { | |
| auto buf = condy::buffer(base_ptr + (to_copy - remaining), remaining); | |
| auto aw1 = condy::async_read( | |
| condy::fixed(0), condy::fixed(buffer_index, buf), chunk_offset); | |
| auto aw2 = condy::async_write( | |
| condy::fixed(1), condy::fixed(buffer_index, buf), chunk_offset); | |
| auto [r1, r2] = co_await (std::move(aw1) >> std::move(aw2)); | |
| if (r1 < 0 || r2 < 0) { | |
| std::fprintf(stderr, "Failed to copy at offset %lld: %d %d\n", | |
| (long long)chunk_offset, r1, r2); | |
| exit(1); | |
| } | |
| if (r1 == 0 || r2 == 0) { | |
| std::fprintf(stderr, | |
| "Unexpected short transfer at offset %lld: %d %d\n", | |
| (long long)chunk_offset, r1, r2); | |
| exit(1); | |
| } | |
| if (r1 != r2) { | |
| std::fprintf(stderr, | |
| "Mismatched read/write at offset %lld: %d %d\n", | |
| (long long)chunk_offset, r1, r2); | |
| exit(1); | |
| } | |
| remaining -= static_cast<loff_t>(r1); | |
| chunk_offset += static_cast<loff_t>(r1); |
| } | ||
|
|
||
| std::string host = argv[1]; | ||
| uint16_t port = static_cast<uint16_t>(std::stoi(argv[2])); |
There was a problem hiding this comment.
The port parsing using std::stoi doesn't handle invalid input gracefully. If the port argument is not a valid integer or is out of the valid port range (0-65535), std::stoi will throw an exception. While the function is marked noexcept(false), it would be better to validate the port number and provide a meaningful error message to the user.
| uint16_t port = static_cast<uint16_t>(std::stoi(argv[2])); | |
| uint16_t port; | |
| try { | |
| std::size_t pos = 0; | |
| int parsed = std::stoi(argv[2], &pos); | |
| // Ensure the whole argument was parsed and the value is within valid port range. | |
| if (argv[2][pos] != '\0' || parsed < 0 || parsed > 65535) { | |
| std::fprintf(stderr, | |
| "Invalid port '%s'. Please provide a number between 0 and 65535.\n", | |
| argv[2]); | |
| return 1; | |
| } | |
| port = static_cast<uint16_t>(parsed); | |
| } catch (const std::exception &) { | |
| std::fprintf(stderr, | |
| "Invalid port '%s'. Please provide a number between 0 and 65535.\n", | |
| argv[2]); | |
| return 1; | |
| } |
| case 't': | ||
| task_num = std::stoul(optarg); | ||
| break; |
There was a problem hiding this comment.
The task_num parsing using std::stoul doesn't validate the input. If optarg is not a valid number or results in an overflow, std::stoul will throw an exception. Additionally, task_num of 0 would cause issues later (division by zero or infinite loops). Consider adding input validation to ensure task_num is a reasonable positive value.
| case 't': | |
| task_num = std::stoul(optarg); | |
| break; | |
| case 't': { | |
| try { | |
| unsigned long value = std::stoul(optarg); | |
| if (value == 0) { | |
| std::fprintf(stderr, | |
| "Invalid task_num '%s': value must be a positive integer\n", | |
| optarg); | |
| usage(argv[0]); | |
| return 1; | |
| } | |
| task_num = static_cast<size_t>(value); | |
| } catch (const std::exception &e) { | |
| std::fprintf(stderr, | |
| "Invalid task_num '%s': %s\n", | |
| optarg, e.what()); | |
| usage(argv[0]); | |
| return 1; | |
| } | |
| break; | |
| } |
| static size_t chunk_size = 1024 * 1024l; // 1 MB | ||
| static bool use_direct = false; | ||
|
|
||
| condy::Coro<void> copy_file_task(size_t task_id, loff_t &offset, |
There was a problem hiding this comment.
The offset parameter is passed by reference and is shared across multiple concurrent tasks. This creates a race condition where multiple tasks can read and modify the same offset variable simultaneously without synchronization. This will lead to unpredictable behavior, data corruption, or missed file chunks during copying.
Consider either using atomic operations for the offset, or use a different approach to distribute work among tasks (e.g., pre-calculate the offset ranges for each task or use a mutex to protect access).
| size_t get_chunk_size(const char *arg) { | ||
| size_t len = std::strlen(arg); | ||
| int suffix = std::tolower(arg[len - 1]); | ||
| size_t multiplier = 1; | ||
| if (suffix == 'k') { | ||
| multiplier = 1024; | ||
| len -= 1; | ||
| } else if (suffix == 'm') { | ||
| multiplier = 1024l * 1024; | ||
| len -= 1; | ||
| } else if (suffix == 'g') { | ||
| multiplier = 1024l * 1024 * 1024; | ||
| len -= 1; | ||
| } | ||
| return std::stoul(std::string(arg, len)) * multiplier; | ||
| } |
There was a problem hiding this comment.
The get_chunk_size function doesn't handle invalid input gracefully. If the string is empty or if std::stoul fails to parse the number, it will throw an exception. Consider adding input validation and error handling for cases where:
- The argument string is empty
- The numeric part is not a valid number
- The result overflows the size_t type
|
|
||
| condy::sync_wait(co_main(host, port)); | ||
| return 0; | ||
| return condy::sync_wait(runtime, co_main(server_fd)); |
There was a problem hiding this comment.
The server_fd socket is never closed before the program exits. If condy::sync_wait returns normally or if an error occurs in co_main, the socket file descriptor will leak. Consider closing the socket after sync_wait returns, or use RAII (e.g., a unique_ptr with a custom deleter) to ensure proper cleanup.
| return condy::sync_wait(runtime, co_main(server_fd)); | |
| int result = condy::sync_wait(runtime, co_main(server_fd)); | |
| close(server_fd); | |
| return result; |
| condy::Coro<int> co_main(int server_fd) { | ||
| while (true) { | ||
| sockaddr_in client_addr; | ||
| socklen_t client_len = sizeof(client_addr); | ||
| int client_fd = co_await condy::async_accept_direct( | ||
| server_fd, (struct sockaddr *)&client_addr, &client_len, 0, | ||
| CONDY_FILE_INDEX_ALLOC); | ||
| if (client_fd < 0) { | ||
| std::fprintf(stderr, "Failed to accept connection: %d\n", | ||
| client_fd); | ||
| co_return 1; | ||
| } | ||
|
|
||
| condy::co_spawn(session(client_fd)).detach(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The co_main function has an infinite loop that never returns under normal circumstances. This means the return value of 1 on line 48 will only be reached if async_accept_direct fails. However, the function is declared to return int, but the infinite loop prevents reaching a normal return path. Consider either documenting that this is intentional behavior (the server runs indefinitely), or implement a shutdown mechanism.
No description provided.