| Message ID | 20180306040220.7519-4-joel@jms.id.au |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Misc fixes | expand |
On Tue, 2018-03-06 at 14:32 +1030, Joel Stanley wrote: > clang errors out about an unused have_busybox function: > > discover/paths.c:44:13: error: unused function 'have_busybox' [-Werror,-Wunused-function] > static bool have_busybox(void) > ^ > > The function is only used once, so move the preprocessor guard down to > the use site instead. This makes it sound like clang is wrong, its single use is inside #ifndef PETITBOOT_TEST which, I assume you must have had defined. Might I suggest moving it down ~30 lines into the #ifndef section? > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > I understand if you don't want to take this one as-is. > > discover/paths.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/discover/paths.c b/discover/paths.c > index 24e978b4e2c1..1f279a103b44 100644 > --- a/discover/paths.c > +++ b/discover/paths.c > @@ -41,15 +41,6 @@ struct load_task { > void *async_data; > }; > > -static inline bool have_busybox(void) > -{ > -#ifdef WITH_BUSYBOX > - return true; > -#else > - return false; > -#endif > -} > - > const char *mount_base(void) > { > return DEVICE_MOUNT_BASE; > @@ -571,8 +562,10 @@ struct load_url_result *load_url_async(void *ctx, struct pb_url *url, > task->process->stdout_data = stdout_data; > } > > - if (!stdout_cb && stdout_data && have_busybox()) > +#ifdef WITH_BUSYBOX > + if (!stdout_cb && stdout_data) > task->process->stdout_cb = busybox_progress_cb; > +#endif Fair warning, doing this gets meeeesssssyyyyyyy. Feel free to have a look at the current and previous versions of arch/powerpc/kernel/process.c in Linux. The current solution over there is to do #ifdef WITH_BUSYBOX static inline bool have_busybox(void) { return true; } #else static inline bool have_busybox(void) { return false; } #endif Somewhere at the top of the file or in a header. > > /* Make sure we save output for any task that has a custom handler */ > if (task->process->stdout_cb) {
On Tue, 2018-03-06 at 16:03 +1100, Cyril Bur wrote: > On Tue, 2018-03-06 at 14:32 +1030, Joel Stanley wrote: > > clang errors out about an unused have_busybox function: > > > > discover/paths.c:44:13: error: unused function 'have_busybox' [-Werror,-Wunused-function] > > static bool have_busybox(void) > > ^ > > > > The function is only used once, so move the preprocessor guard down to > > the use site instead. > > This makes it sound like clang is wrong, its single use is inside > #ifndef PETITBOOT_TEST which, I assume you must have had defined. > > Might I suggest moving it down ~30 lines into the #ifndef section? > > > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > --- > > I understand if you don't want to take this one as-is. > > > > discover/paths.c | 13 +++---------- > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > > diff --git a/discover/paths.c b/discover/paths.c > > index 24e978b4e2c1..1f279a103b44 100644 > > --- a/discover/paths.c > > +++ b/discover/paths.c > > @@ -41,15 +41,6 @@ struct load_task { > > void *async_data; > > }; > > > > -static inline bool have_busybox(void) > > -{ > > -#ifdef WITH_BUSYBOX > > - return true; > > -#else > > - return false; > > -#endif > > -} > > - > > const char *mount_base(void) > > { > > return DEVICE_MOUNT_BASE; > > @@ -571,8 +562,10 @@ struct load_url_result *load_url_async(void *ctx, struct pb_url *url, > > task->process->stdout_data = stdout_data; > > } > > > > - if (!stdout_cb && stdout_data && have_busybox()) > > +#ifdef WITH_BUSYBOX > > + if (!stdout_cb && stdout_data) > > task->process->stdout_cb = busybox_progress_cb; > > +#endif > > Fair warning, doing this gets meeeesssssyyyyyyy. Feel free to have a > look at the current and previous versions of > arch/powerpc/kernel/process.c in Linux. > > The current solution over there is to do > #ifdef WITH_BUSYBOX > static inline bool have_busybox(void) { return true; } > #else > static inline bool have_busybox(void) { return false; } > #endif > > Somewhere at the top of the file or in a header. This is probably the way to go - otherwise with the original patch we get 13:46:09 discover/paths.c:135:12: warning: ‘busybox_progress_cb’ defined but not used [-Wunused-function] 13:46:09 static int busybox_progress_cb(void *arg) which is no fun. > > > > > /* Make sure we save output for any task that has a custom handler */ > > if (task->process->stdout_cb) { > > _______________________________________________ > Petitboot mailing list > Petitboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/petitboot
On Wed, 2018-03-07 at 13:49 +1100, Samuel Mendoza-Jonas wrote: > On Tue, 2018-03-06 at 16:03 +1100, Cyril Bur wrote: > > On Tue, 2018-03-06 at 14:32 +1030, Joel Stanley wrote: > > > clang errors out about an unused have_busybox function: > > > > > > discover/paths.c:44:13: error: unused function 'have_busybox' [-Werror,-Wunused-function] > > > static bool have_busybox(void) > > > ^ > > > > > > The function is only used once, so move the preprocessor guard down to > > > the use site instead. > > > > This makes it sound like clang is wrong, its single use is inside > > #ifndef PETITBOOT_TEST which, I assume you must have had defined. > > > > Might I suggest moving it down ~30 lines into the #ifndef section? > > > > > > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > > --- > > > I understand if you don't want to take this one as-is. > > > > > > discover/paths.c | 13 +++---------- > > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > > > > diff --git a/discover/paths.c b/discover/paths.c > > > index 24e978b4e2c1..1f279a103b44 100644 > > > --- a/discover/paths.c > > > +++ b/discover/paths.c > > > @@ -41,15 +41,6 @@ struct load_task { > > > void *async_data; > > > }; > > > > > > -static inline bool have_busybox(void) > > > -{ > > > -#ifdef WITH_BUSYBOX > > > - return true; > > > -#else > > > - return false; > > > -#endif > > > -} > > > - > > > const char *mount_base(void) > > > { > > > return DEVICE_MOUNT_BASE; > > > @@ -571,8 +562,10 @@ struct load_url_result *load_url_async(void *ctx, struct pb_url *url, > > > task->process->stdout_data = stdout_data; > > > } > > > > > > - if (!stdout_cb && stdout_data && have_busybox()) > > > +#ifdef WITH_BUSYBOX > > > + if (!stdout_cb && stdout_data) > > > task->process->stdout_cb = busybox_progress_cb; > > > +#endif > > > > Fair warning, doing this gets meeeesssssyyyyyyy. Feel free to have a > > look at the current and previous versions of > > arch/powerpc/kernel/process.c in Linux. > > > > The current solution over there is to do > > #ifdef WITH_BUSYBOX > > static inline bool have_busybox(void) { return true; } > > #else > > static inline bool have_busybox(void) { return false; } > > #endif > > > > Somewhere at the top of the file or in a header. > > This is probably the way to go - otherwise with the original patch we get > > 13:46:09 discover/paths.c:135:12: warning: ‘busybox_progress_cb’ defined > but not used [-Wunused-function] > 13:46:09 static int busybox_progress_cb(void *arg) > > which is no fun. Or if you have another coffee, technically unrelated and something I should also fix up :) > > > > > > > > > /* Make sure we save output for any task that has a custom handler */ > > > if (task->process->stdout_cb) { > > > > _______________________________________________ > > Petitboot mailing list > > Petitboot@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/petitboot > > _______________________________________________ > Petitboot mailing list > Petitboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/petitboot
diff --git a/discover/paths.c b/discover/paths.c index 24e978b4e2c1..1f279a103b44 100644 --- a/discover/paths.c +++ b/discover/paths.c @@ -41,15 +41,6 @@ struct load_task { void *async_data; }; -static inline bool have_busybox(void) -{ -#ifdef WITH_BUSYBOX - return true; -#else - return false; -#endif -} - const char *mount_base(void) { return DEVICE_MOUNT_BASE; @@ -571,8 +562,10 @@ struct load_url_result *load_url_async(void *ctx, struct pb_url *url, task->process->stdout_data = stdout_data; } - if (!stdout_cb && stdout_data && have_busybox()) +#ifdef WITH_BUSYBOX + if (!stdout_cb && stdout_data) task->process->stdout_cb = busybox_progress_cb; +#endif /* Make sure we save output for any task that has a custom handler */ if (task->process->stdout_cb) {
clang errors out about an unused have_busybox function: discover/paths.c:44:13: error: unused function 'have_busybox' [-Werror,-Wunused-function] static bool have_busybox(void) ^ The function is only used once, so move the preprocessor guard down to the use site instead. Signed-off-by: Joel Stanley <joel@jms.id.au> --- I understand if you don't want to take this one as-is. discover/paths.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)