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.

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

Fully initialize custom_operations job_ops #918

Merged

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Jan 5, 2022

It was missing an initializer for .compare_ext (since 3.12.1), the
default is custom_compare_ext_default; and .fixed_length (since
4.08.0) for which NULL is good.

@raphael-proust
Copy link
Collaborator

I don't know enough about this part of the code or the intricacies of the underlying system to meaningfully review this code.

With that in mind, my very generic questions are:

  • Do we need to change src/unix/lwt_libev_stubs.c where loop_ops and watcher_ops do use this struct?
  • Should we add a test with a custom job to the unix test suite?

@MisterDA
Copy link
Contributor Author

I've seen the issue when I added warning flags to the C compiler which reported fields missing initializers.
The structs watcher_ops and loop_ops are static, so the fields are implicitly initialized to NULL by the compiler, which is not the case for job_ops as it isn't static (as far as my knowledge of C goes).
The fact that we never had a crash related to garbage values for these fields in job_ops makes me think that they're not used...

Do we need to change src/unix/lwt_libev_stubs.c where loop_ops and watcher_ops do use this struct?

Seems the best to me, I missed them because the compiler didn't report a warning.

Should we add a test with a custom job to the unix test suite?

Testing never hurts!

@MisterDA
Copy link
Contributor Author

  • Do we need to change src/unix/lwt_libev_stubs.c where loop_ops and watcher_ops do use this struct?

Done.

  • Should we add a test with a custom job to the unix test suite?

I don't think it's necessary because we're already using all the default functions which raise Failure when called, and the new one do too. I don't know what this test would look like.

@MisterDA MisterDA force-pushed the initialize-custom_operations branch from 1cb9aad to 8dacb11 Compare June 21, 2022 08:18
It was missing an initializer for `.compare_ext` (since 3.12.1), the
default is `custom_compare_ext_default`; and `.fixed_length` (since
4.08.0) for which `NULL` is good.
@MisterDA MisterDA force-pushed the initialize-custom_operations branch from 8dacb11 to 61067c4 Compare June 21, 2022 08:27
@raphael-proust raphael-proust merged commit b533a46 into ocsigen:master Jun 22, 2022
@MisterDA MisterDA deleted the initialize-custom_operations branch June 22, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants