Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat common: slight code improvement #9

Closed
wants to merge 3 commits into from
Closed

feat common: slight code improvement #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f30b414
cmake: build tests
May 8, 2024
1cc984a
tests: use a thread-safe function to transform time
May 13, 2024
2cee9b2
build: fixing unused-result warning
May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ jobs:
- name: Install libraries
run: |
sudo apt update
sudo apt install cmake ninja-build clang
sudo apt install cmake ninja-build clang libaio-dev

- name: Build
run: |
Expand Down
6 changes: 4 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
build/
subprojects/libblkio/
.*
/build_*/
/build/
/subprojects/libblkio/
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ message("Libvhost log verbosity: ${LIBVHOST_LOG_VERBOSITY}")
add_library(vhost-server)
add_compile_definitions(_GNU_SOURCE LOG_VERBOSITY=${LIBVHOST_LOG_VERBOSITY})
target_compile_options(vhost-server PRIVATE
-std=gnu17
Copy link
Collaborator

Choose a reason for hiding this comment

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

This CMakeLists.txt was introduced by NBS since they integrate libvhost as part of their project, we don't actually build with it locally.
This will need to be checked in with them (might break their build).

Copy link
Author

Choose a reason for hiding this comment

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

this code is compiled exclusively by gnu extensions of the standard, it is worth explicitly denoting this

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aikuchin Would you be okay with this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need 17 here? I was quite sure that 11 was enough, maybe 14, but 17 seems like an overkill and will limit supported compilers significantly.

Copy link
Author

Choose a reason for hiding this comment

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

C14 does not exist. C17 is supported by GCC 8.1 and Clang 7. C23 causes confusion.

Copy link
Contributor

@aikuchin aikuchin Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to understand the motivation for gnu17 if we only use features for gnu11. Why do we need 17, do we ?
If we need new compiler features it is easy to bump requirements in new version, but downgrading standard is always more complicated.
UPD: Since c17 is just a bugfix release for c11 I agree that gnu17 is OK here.

And should we also add required standard version to meson.build?

-Wall
-Werror
-Wextra
Expand Down Expand Up @@ -61,3 +62,6 @@ target_sources(vhost-server PRIVATE
virtio/virtio_blk.c
virtio/virtio_fs.c
)


add_subdirectory(tests)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, I'm not sure NBS needs the tests subdirectory for their integration (and this might break their isolated build as well)

Copy link
Author

Choose a reason for hiding this comment

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

It is very unusual to be selective about build systems and focus on only one consumer for an open source project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Building tests makes sense and NBS will fix their build if building tests break it. So I think this is a good idea to build tests.

23 changes: 23 additions & 0 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
find_library(LIBAIO NAMES aio REQUIRED)

add_executable(vhost-user-blk-test-server vhost_user_blk_test_server.c)
target_include_directories(vhost-user-blk-test-server
PRIVATE
./
../ # TODO: remove external directories
)
target_link_libraries(vhost-user-blk-test-server
vhost-server
${LIBAIO}
)
target_compile_options(vhost-user-blk-test-server PRIVATE
-std=gnu17
-Wall
-Werror
-Wextra
-Wno-unused-parameter
-g
-O2

-Wno-error=unused-result
)
3 changes: 2 additions & 1 deletion tests/test_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ static inline void vhd_log_stderr(enum LogLevel level, const char *fmt, ...)
if (level <= LOG_VERBOSITY) {
char timestr[64];
struct timeval tv;
struct tm local_tm;

gettimeofday(&tv, NULL);
strftime(timestr, sizeof(timestr), "%F %T", localtime(&tv.tv_sec));
strftime(timestr, sizeof(timestr), "%F %T", localtime_r(&tv.tv_sec, &local_tm));
fprintf(stderr, "%s.%03ld [%8s] ", timestr, tv.tv_usec / 1000,
log_level_str[level]);
vfprintf(stderr, fmt, args);
Expand Down
3 changes: 2 additions & 1 deletion tests/vhost_user_blk_test_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,8 @@ static void monitor_serve_unix_socket(const char *path,
static void interactive_sigint(int sig)
{
const char *const msg = "\nUse 'stop' command or Ctrl+D to exit\n";
write(2, msg, strlen(msg));
ssize_t res = write(2, msg, strlen(msg));
VHD_ASSERT(res == strlen(msg));
algol83 marked this conversation as resolved.
Show resolved Hide resolved
}

static void monitor_serve_stdio(struct disks_context *ctx)
Expand Down
3 changes: 2 additions & 1 deletion vdev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1940,7 +1940,8 @@ static int timer_read(void *opaque)
uint64_t count;

/* Read the count to rearm the periodic timer, but ignore the result */
algol83 marked this conversation as resolved.
Show resolved Hide resolved
(void)read(vdev->timerfd, &count, sizeof(count));
ssize_t res = read(vdev->timerfd, &count, sizeof(count));
VHD_ASSERT(res == sizeof(count));

elapsed_time(vdev, &elapsed);

Expand Down
Loading