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

Rename generic error function #15296

Merged
merged 4 commits into from Jul 6, 2023
Merged

Conversation

thiagoftsm
Copy link
Contributor

Summary

This PR proceeds with changes started when we merge PR #15266.

The reasons are the same that other PR was made.

Test Plan
  1. Compile this branch and let it running.
  2. Now review if nothing is missed running inside branch.
$ grep -r "error(" *| less

We should have netdata running and our code (please, ignore submodules) should not have any error function.

Additional Information

To make this PR I used CLion to rename functions, and after this I used grep and git diff to check that everything was renamed properly.

For users: How does this change affect me? Describe the PR affects users: - Which area of Netdata is affected by the change? the whole code. - Can they see the change or is it an under the hood? If they can see it, where? No, this affects more package maintainers. - How is the user impacted by the change? They will have maintainers releasing packages faster. - What are there any benefits of the change? We will not affect other software compilation.

Copy link
Contributor

@Dim-P Dim-P left a comment

Choose a reason for hiding this comment

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

Hey @thiagoftsm . What about the other functions here:

#ifdef NETDATA_INTERNAL_CHECKS
#define debug(type, args...) do { if(unlikely(debug_flags & type)) debug_int(__FILE__, __FUNCTION__, __LINE__, ##args); } while(0)
#define internal_error(condition, args...) do { if(unlikely(condition)) error_int(0, "IERR", __FILE__, __FUNCTION__, __LINE__, ##args); } while(0)
#define internal_fatal(condition, args...) do { if(unlikely(condition)) fatal_int(__FILE__, __FUNCTION__, __LINE__, ##args); } while(0)
#else
#define debug(type, args...) debug_dummy()
#define internal_error(args...) debug_dummy()
#define internal_fatal(args...) debug_dummy()
#endif

#define netdata_log_info(args...)    info_int(0, __FILE__, __FUNCTION__, __LINE__, ##args)
#define collector_info(args...)    info_int(1, __FILE__, __FUNCTION__, __LINE__, ##args)
#define infoerr(args...) error_int(0, "INFO", __FILE__, __FUNCTION__, __LINE__, ##args)
#define netdata_log_error(args...)   error_int(0, "ERROR", __FILE__, __FUNCTION__, __LINE__, ##args)
#define collector_infoerr(args...) error_int(1, "INFO", __FILE__, __FUNCTION__, __LINE__, ##args)
#define collector_error(args...)   error_int(1, "ERROR", __FILE__, __FUNCTION__, __LINE__, ##args)
#define error_limit(erl, args...)   error_limit_int(erl, "ERROR", __FILE__, __FUNCTION__, __LINE__, ##args)
#define fatal(args...)   fatal_int(__FILE__, __FUNCTION__, __LINE__, ##args)
#define fatal_assert(expr) ((expr) ? (void)(0) : fatal_int(__FILE__, __FUNCTION__, __LINE__, "Assertion `%s' failed", #expr))

I would argue we need to at least change debug() to netdata_log_debug() and infoerr() to netdata_log_infoerr(). Not sure about the collector logging functions.

BTW, infoerr() is used only once in ebpf.c, so it could be remove altogether.

@thiagoftsm
Copy link
Contributor Author

Hello @Dim-P ,

I agree 100% with you, but probably it is better to bring a PR per function to simplify reviews. This PR has almost 2000 changes, so I do not want to make it even bigger. After we merge this, I will address the others. 🤝

Best regards!

@Dim-P
Copy link
Contributor

Dim-P commented Jul 6, 2023

Hello @Dim-P ,

I agree 100% with you, but probably it is better to bring a PR per function to simplify reviews. This PR has almost 2000 changes, so I do not want to make it even bigger. After we merge this, I will address the others. 🤝

Best regards!

OK, that makes sense. There are some conflicts at the moment, if you can rebase it once more, I can get it reviewed today.

Copy link
Contributor

@Dim-P Dim-P left a comment

Choose a reason for hiding this comment

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

@thiagoftsm Please also change these three:

collectors/ebpf.plugin/ebpf_disk.c: In function 'ebpf_disk_load_bpf':                                                                                                                                                                         
collectors/ebpf.plugin/ebpf_disk.c:817:9: warning: implicit declaration of function 'error'; did you mean 'herror'? [-Wimplicit-function-declaration]                                                                                         
  817 |         error("%s %s", EBPF_DEFAULT_ERROR_MSG, em->thread_name);                                                                                                                                                                      
      |         ^~~~~                                                                                                                                                                                                                         
      |         herror      
collectors/ebpf.plugin/ebpf_mdflush.c: In function 'ebpf_mdflush_thread':                                                                                                                                                                     
collectors/ebpf.plugin/ebpf_mdflush.c:396:9: warning: implicit declaration of function 'error'; did you mean 'herror'? [-Wimplicit-function-declaration]                                                                                      
  396 |         error("Cannot load eBPF software.");                                                                                                                                                                                          
      |         ^~~~~                                                                                                                                                                                                                         
      |         herror  
database/rrdhost.c: In function 'dbengine_init':
database/rrdhost.c:980:9: warning: implicit declaration of function 'error'; did you mean 'herror'? [-Wimplicit-function-declaration]
  980 |         error("DBENGINE is not available on '%s', so only 1 database tier can be supported.", hostname);
      |         ^~~~~
      |         herror

And there is another one too which is commented out at streaming/sender.c:584:

    if(unlikely(s->rrdpush_sender_socket == -1)) {
        // error("STREAM %s [send to %s]: could not connect to parent node at this time.", rrdhost_hostname(host), host->rrdpush_send_destination);
        return false;
    }

@thiagoftsm thiagoftsm requested a review from Dim-P July 6, 2023 13:10
@thiagoftsm
Copy link
Contributor Author

@thiagoftsm Please also change these three:

Very interesting, I did not have these issues while was running a software that I use to develop 🤦 .
Thank you!

Copy link
Contributor

@Dim-P Dim-P left a comment

Choose a reason for hiding this comment

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

All good now!

@thiagoftsm thiagoftsm merged commit e0f388c into netdata:master Jul 6, 2023
129 checks passed
@thiagoftsm thiagoftsm deleted the rename_error_fcnt branch July 6, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants