| Message ID | 20210322142912.26521-1-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtracefs: Prefer using const pointer arguments | expand |
On Mon, 22 Mar 2021 16:29:12 +0200 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > All functions should receive const pointer arguments, except in the > cases when the argument object itself needs to be modified. > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > include/tracefs.h | 15 ++++++++------- > src/tracefs-instance.c | 17 +++++++++-------- > 2 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/include/tracefs.h b/include/tracefs.h > index f3eec62..e54f791 100644 > --- a/include/tracefs.h > +++ b/include/tracefs.h > @@ -25,12 +25,13 @@ struct tracefs_instance *tracefs_instance_create(const char *name); > struct tracefs_instance *tracefs_instance_alloc(const char *tracing_dir, > const char *name); > int tracefs_instance_destroy(struct tracefs_instance *instance); > -bool tracefs_instance_is_new(struct tracefs_instance *instance); > -const char *tracefs_instance_get_name(struct tracefs_instance *instance); > -const char *tracefs_instance_get_trace_dir(struct tracefs_instance *instance); > +bool tracefs_instance_is_new(const struct tracefs_instance *instance); > +const char *tracefs_instance_get_name(const struct tracefs_instance *instance); > +const char *tracefs_instance_get_trace_dir(const struct tracefs_instance *instance); > char * > -tracefs_instance_get_file(struct tracefs_instance *instance, const char *file); > -char *tracefs_instance_get_dir(struct tracefs_instance *instance); > +tracefs_instance_get_file(const struct tracefs_instance *instance, > + const char *file); > +char *tracefs_instance_get_dir(const struct tracefs_instance *instance); > int tracefs_instance_file_write(struct tracefs_instance *instance, > const char *file, const char *str); > char *tracefs_instance_file_read(struct tracefs_instance *instance, > @@ -42,8 +43,8 @@ int tracefs_instance_file_open(struct tracefs_instance *instance, > int tracefs_instances_walk(int (*callback)(const char *, void *), void *context); > > bool tracefs_instance_exists(const char *name); > -bool tracefs_file_exists(struct tracefs_instance *instance, char *name); > -bool tracefs_dir_exists(struct tracefs_instance *instance, char *name); > +bool tracefs_file_exists(struct tracefs_instance *instance, const char *name); > +bool tracefs_dir_exists(struct tracefs_instance *instance, const char *name); > > int tracefs_trace_is_on(struct tracefs_instance *instance); > int tracefs_trace_on(struct tracefs_instance *instance); > My fear about doing the above is that when dealing with complex structures like tracefs_instance, which is opaque (private) to the caller that it now prevents us from ever modifying the instance in one of those APIs. It may not modify it now, but that does not mean it wont be modified in the future. We could add ref counters, or something else. What's the reason to make them constant? The caller does not have access to the internals of what those are pointing to. It's just an abstract object. Why would they care if its const or not? -- Steve
On 22.03.21 г. 17:47, Steven Rostedt wrote: > On Mon, 22 Mar 2021 16:29:12 +0200 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> All functions should receive const pointer arguments, except in the >> cases when the argument object itself needs to be modified. >> >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> include/tracefs.h | 15 ++++++++------- >> src/tracefs-instance.c | 17 +++++++++-------- >> 2 files changed, 17 insertions(+), 15 deletions(-) >> >> diff --git a/include/tracefs.h b/include/tracefs.h >> index f3eec62..e54f791 100644 >> --- a/include/tracefs.h >> +++ b/include/tracefs.h >> @@ -25,12 +25,13 @@ struct tracefs_instance *tracefs_instance_create(const char *name); >> struct tracefs_instance *tracefs_instance_alloc(const char *tracing_dir, >> const char *name); >> int tracefs_instance_destroy(struct tracefs_instance *instance); >> -bool tracefs_instance_is_new(struct tracefs_instance *instance); >> -const char *tracefs_instance_get_name(struct tracefs_instance *instance); >> -const char *tracefs_instance_get_trace_dir(struct tracefs_instance *instance); >> +bool tracefs_instance_is_new(const struct tracefs_instance *instance); >> +const char *tracefs_instance_get_name(const struct tracefs_instance *instance); >> +const char *tracefs_instance_get_trace_dir(const struct tracefs_instance *instance); >> char * >> -tracefs_instance_get_file(struct tracefs_instance *instance, const char *file); >> -char *tracefs_instance_get_dir(struct tracefs_instance *instance); >> +tracefs_instance_get_file(const struct tracefs_instance *instance, >> + const char *file); >> +char *tracefs_instance_get_dir(const struct tracefs_instance *instance); >> int tracefs_instance_file_write(struct tracefs_instance *instance, >> const char *file, const char *str); >> char *tracefs_instance_file_read(struct tracefs_instance *instance, >> @@ -42,8 +43,8 @@ int tracefs_instance_file_open(struct tracefs_instance *instance, >> int tracefs_instances_walk(int (*callback)(const char *, void *), void *context); >> >> bool tracefs_instance_exists(const char *name); >> -bool tracefs_file_exists(struct tracefs_instance *instance, char *name); >> -bool tracefs_dir_exists(struct tracefs_instance *instance, char *name); >> +bool tracefs_file_exists(struct tracefs_instance *instance, const char *name); >> +bool tracefs_dir_exists(struct tracefs_instance *instance, const char *name); >> >> int tracefs_trace_is_on(struct tracefs_instance *instance); >> int tracefs_trace_on(struct tracefs_instance *instance); >> > > My fear about doing the above is that when dealing with complex structures > like tracefs_instance, which is opaque (private) to the caller that it now > prevents us from ever modifying the instance in one of those APIs. It may > not modify it now, but that does not mean it wont be modified in the > future. We could add ref counters, or something else. > Hi Steven, I understand your preference to keep the flexibility of those API, however generally spiking those are "getter" functions and they are not supposed to modify the encapsulated object. If we allow the "getters" of the API to modify the encapsulated object (tracefs_instancein this case) this will be a bit of hacking stile of programing. At least for me, having a firm and intuitive design of the API is more valuable than the inconvenience that this reduced flexibility can potentially bring. Thanks! Yordan > What's the reason to make them constant? The caller does not have access to > the internals of what those are pointing to. It's just an abstract object. > Why would they care if its const or not? > > -- Steve >
On Mon, 22 Mar 2021 18:59:42 +0200 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > I understand your preference to keep the flexibility of those API, > however generally spiking those are "getter" functions and they are not > supposed to modify the encapsulated object. If we allow the "getters" of > the API to modify the encapsulated object (tracefs_instancein this case) > this will be a bit of hacking stile of programing. At least for me, > having a firm and intuitive design of the API is more valuable than the > inconvenience that this reduced flexibility can potentially bring. Is there an immediate need for this change? I.e. python complains about it, or some other reason. I'd like to hold off on changing this. This shouldn't break APIs, as the code should still link fine if we decide to change it later. I looked at various other libraries to see if they use const pointers for "getter" functions. It's somewhat inconsistent. I'll look at more libraries, and see how they are done. I'm more interested in keeping with precedent here. C does some things differently that C++. -- Steve
On 23.03.21 г. 0:29, Steven Rostedt wrote: > On Mon, 22 Mar 2021 18:59:42 +0200 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> I understand your preference to keep the flexibility of those API, >> however generally spiking those are "getter" functions and they are not >> supposed to modify the encapsulated object. If we allow the "getters" of >> the API to modify the encapsulated object (tracefs_instancein this case) >> this will be a bit of hacking stile of programing. At least for me, >> having a firm and intuitive design of the API is more valuable than the >> inconvenience that this reduced flexibility can potentially bring. > > Is there an immediate need for this change? I.e. python complains about it, > or some other reason. Hi Steven, I really need to change all "char *" arguments to "const char *". Changing "tracefs_instance *" to "const tracefs_instance *" is more a style preference. Thanks a lot! Y. > > I'd like to hold off on changing this. This shouldn't break APIs, as the > code should still link fine if we decide to change it later. > > I looked at various other libraries to see if they use const pointers for > "getter" functions. It's somewhat inconsistent. I'll look at more > libraries, and see how they are done. I'm more interested in keeping with > precedent here. C does some things differently that C++. > > -- Steve >
On Tue, 23 Mar 2021 14:58:07 +0200 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > I really need to change all "char *" arguments to "const char *". > Changing "tracefs_instance *" to "const tracefs_instance *" is more a > style preference. I totally agree on the "char *" to "const char *", as that can make a huge difference. I'm not adverse to the "const struct tracefs_instance *" change, I'm just hesitant due to my years of torment by non flexible APIs ;-) -- Steve
diff --git a/include/tracefs.h b/include/tracefs.h index f3eec62..e54f791 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -25,12 +25,13 @@ struct tracefs_instance *tracefs_instance_create(const char *name); struct tracefs_instance *tracefs_instance_alloc(const char *tracing_dir, const char *name); int tracefs_instance_destroy(struct tracefs_instance *instance); -bool tracefs_instance_is_new(struct tracefs_instance *instance); -const char *tracefs_instance_get_name(struct tracefs_instance *instance); -const char *tracefs_instance_get_trace_dir(struct tracefs_instance *instance); +bool tracefs_instance_is_new(const struct tracefs_instance *instance); +const char *tracefs_instance_get_name(const struct tracefs_instance *instance); +const char *tracefs_instance_get_trace_dir(const struct tracefs_instance *instance); char * -tracefs_instance_get_file(struct tracefs_instance *instance, const char *file); -char *tracefs_instance_get_dir(struct tracefs_instance *instance); +tracefs_instance_get_file(const struct tracefs_instance *instance, + const char *file); +char *tracefs_instance_get_dir(const struct tracefs_instance *instance); int tracefs_instance_file_write(struct tracefs_instance *instance, const char *file, const char *str); char *tracefs_instance_file_read(struct tracefs_instance *instance, @@ -42,8 +43,8 @@ int tracefs_instance_file_open(struct tracefs_instance *instance, int tracefs_instances_walk(int (*callback)(const char *, void *), void *context); bool tracefs_instance_exists(const char *name); -bool tracefs_file_exists(struct tracefs_instance *instance, char *name); -bool tracefs_dir_exists(struct tracefs_instance *instance, char *name); +bool tracefs_file_exists(struct tracefs_instance *instance, const char *name); +bool tracefs_dir_exists(struct tracefs_instance *instance, const char *name); int tracefs_trace_is_on(struct tracefs_instance *instance); int tracefs_trace_on(struct tracefs_instance *instance); diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c index 0df4e13..ba467a8 100644 --- a/src/tracefs-instance.c +++ b/src/tracefs-instance.c @@ -100,7 +100,7 @@ out: * Returns true, if the ftrace instance is newly created by the library or * false otherwise. */ -bool tracefs_instance_is_new(struct tracefs_instance *instance) +bool tracefs_instance_is_new(const struct tracefs_instance *instance) { if (instance && (instance->flags & FLAG_INSTANCE_NEWLY_CREATED)) return true; @@ -229,7 +229,8 @@ int tracefs_instance_destroy(struct tracefs_instance *instance) * Must use tracefs_put_tracing_file() to free the returned string. */ char * -tracefs_instance_get_file(struct tracefs_instance *instance, const char *file) +tracefs_instance_get_file(const struct tracefs_instance *instance, + const char *file) { char *path = NULL; int ret; @@ -255,7 +256,7 @@ tracefs_instance_get_file(struct tracefs_instance *instance, const char *file) * * Must use tracefs_put_tracing_file() to free the returned string. */ -char *tracefs_instance_get_dir(struct tracefs_instance *instance) +char *tracefs_instance_get_dir(const struct tracefs_instance *instance) { char *path = NULL; int ret; @@ -283,7 +284,7 @@ char *tracefs_instance_get_dir(struct tracefs_instance *instance) * Returns the name of the given @instance. * The returned string must *not* be freed. */ -const char *tracefs_instance_get_name(struct tracefs_instance *instance) +const char *tracefs_instance_get_name(const struct tracefs_instance *instance) { if (instance) return instance->name; @@ -297,7 +298,7 @@ const char *tracefs_instance_get_name(struct tracefs_instance *instance) * Returns the top trace directory where the given @instance is configured. * The returned string must *not* be freed. */ -const char *tracefs_instance_get_trace_dir(struct tracefs_instance *instance) +const char *tracefs_instance_get_trace_dir(const struct tracefs_instance *instance) { if (instance) return instance->trace_dir; @@ -436,7 +437,7 @@ int tracefs_instance_file_open(struct tracefs_instance *instance, } static bool check_file_exists(struct tracefs_instance *instance, - char *name, bool dir) + const char *name, bool dir) { char file[PATH_MAX]; struct stat st; @@ -482,7 +483,7 @@ bool tracefs_instance_exists(const char *name) * * If a directory with the given name exists, false is returned. */ -bool tracefs_file_exists(struct tracefs_instance *instance, char *name) +bool tracefs_file_exists(struct tracefs_instance *instance, const char *name) { return check_file_exists(instance, name, false); } @@ -494,7 +495,7 @@ bool tracefs_file_exists(struct tracefs_instance *instance, char *name) * * Returns true if the directory exists, false otherwise */ -bool tracefs_dir_exists(struct tracefs_instance *instance, char *name) +bool tracefs_dir_exists(struct tracefs_instance *instance, const char *name) { return check_file_exists(instance, name, true); }
All functions should receive const pointer arguments, except in the cases when the argument object itself needs to be modified. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- include/tracefs.h | 15 ++++++++------- src/tracefs-instance.c | 17 +++++++++-------- 2 files changed, 17 insertions(+), 15 deletions(-)