| Message ID | 1348493284-9375-1-git-send-email-rajagopal.venkat@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/24/2012 06:28 AM, Rajagopal Venkat wrote: > This patch adds following minor changes to prepare powertop > to support Android platform. > > - Add missing HAVE_CONFIG_H conditional check. > - remove un-used ethtool_cmd_speed_set and ethtool_cmd_speed > functions. > - Minimize dependency on exception handling in catch blocks. > > These changes will not affect powertop functionality. > > Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> > --- > src/devices/ahci.cpp | 4 ++-- > src/devices/alsa.cpp | 4 ++-- > src/devices/network.cpp | 16 ---------------- > src/main.cpp | 2 ++ > 4 files changed, 6 insertions(+), 20 deletions(-) > > diff --git a/src/devices/ahci.cpp b/src/devices/ahci.cpp > index 1fe39c7..67ce06e 100644 > --- a/src/devices/ahci.cpp > +++ b/src/devices/ahci.cpp > @@ -170,7 +170,7 @@ void ahci::start_measurement(void) > file.close(); > } > catch (std::ios_base::failure &c) { > - fprintf(stderr, "%s\n", c.what()); > + fprintf(stderr, "Failed to start measurement for ahci device\n"); adding addition message here is acceptable, but eliminating the information from the catches error is not. > } > > } > @@ -203,7 +203,7 @@ void ahci::end_measurement(void) > file.close(); > } > catch (std::ios_base::failure &c) { > - fprintf(stderr, "%s\n", c.what()); > + fprintf(stderr, "Failed to end measurement for ahci device\n"); > } > if (end_active < start_active) > end_active = start_active; > diff --git a/src/devices/alsa.cpp b/src/devices/alsa.cpp > index 4f5d3f9..a67780c 100644 > --- a/src/devices/alsa.cpp > +++ b/src/devices/alsa.cpp > @@ -104,7 +104,7 @@ void alsa::start_measurement(void) > file.close(); > } > catch (std::ios_base::failure &c) { > - fprintf(stderr, "%s\n", c.what()); > + fprintf(stderr, "Failed to start measurement for alsa device\n"); > } > } > > @@ -130,7 +130,7 @@ void alsa::end_measurement(void) > file.close(); > } > catch (std::ios_base::failure &c) { > - fprintf(stderr, "%s\n", c.what()); > + fprintf(stderr, "Failed to end measurement for alsa device\n"); > } > > p = (end_active - start_active) / (0.001 + end_active + end_inactive - start_active - start_inactive) * 100.0; > diff --git a/src/devices/network.cpp b/src/devices/network.cpp > index b8a5c9c..ed9d7aa 100644 > --- a/src/devices/network.cpp > +++ b/src/devices/network.cpp > @@ -55,22 +55,6 @@ extern "C" { > > static map<string, class network *> nics; > > -#ifdef DISABLE_TRYCATCH > - > -static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep, > - __u32 speed) > -{ > - > - ep->speed = (__u16)speed; > - ep->speed_hi = (__u16)(speed >> 16); > -} > - > -static inline __u32 ethtool_cmd_speed(struct ethtool_cmd *ep) > -{ > - return (ep->speed_hi << 16) | ep->speed; > -} > - > -#endif > > static void do_proc_net_dev(void) > { > diff --git a/src/main.cpp b/src/main.cpp > index 1815075..dc49dba 100644 > --- a/src/main.cpp > +++ b/src/main.cpp > @@ -42,7 +42,9 @@ > #include "perf/perf.h" > #include "perf/perf_bundle.h" > #include "lib.h" > +#ifdef HAVE_CONFIG_H > #include "../config.h" > +#endif > > > #include "devices/device.h"
On 24 September 2012 21:27, Chris Ferron <chris.e.ferron@linux.intel.com> wrote: > On 09/24/2012 06:28 AM, Rajagopal Venkat wrote: >> >> This patch adds following minor changes to prepare powertop >> to support Android platform. >> >> - Add missing HAVE_CONFIG_H conditional check. >> - remove un-used ethtool_cmd_speed_set and ethtool_cmd_speed >> functions. >> - Minimize dependency on exception handling in catch blocks. >> >> These changes will not affect powertop functionality. >> >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> >> --- >> src/devices/ahci.cpp | 4 ++-- >> src/devices/alsa.cpp | 4 ++-- >> src/devices/network.cpp | 16 ---------------- >> src/main.cpp | 2 ++ >> 4 files changed, 6 insertions(+), 20 deletions(-) >> >> diff --git a/src/devices/ahci.cpp b/src/devices/ahci.cpp >> index 1fe39c7..67ce06e 100644 >> --- a/src/devices/ahci.cpp >> +++ b/src/devices/ahci.cpp >> @@ -170,7 +170,7 @@ void ahci::start_measurement(void) >> file.close(); >> } >> catch (std::ios_base::failure &c) { >> - fprintf(stderr, "%s\n", c.what()); >> + fprintf(stderr, "Failed to start measurement for ahci >> device\n"); > > adding addition message here is acceptable, but eliminating the information > from the catches error is not. > As discussed in first patch set, android doesn't support exception handling. This is the reason powertop had DISABLE_TRYCATCH conditional macro which is removed in recent commit. The patch 2/2 adds stubs for exception handling #define try if (true) #define catch(x) if (false) With this, fprintf(stderr, "%s\n", c.what()); in catch block throws undefined reference to c. So added message instead of c.what(). Any better ways of adding stubs are welcome. > >> } >> } >> @@ -203,7 +203,7 @@ void ahci::end_measurement(void) >> file.close(); >> } >> catch (std::ios_base::failure &c) { >> - fprintf(stderr, "%s\n", c.what()); >> + fprintf(stderr, "Failed to end measurement for ahci >> device\n"); >> } >> if (end_active < start_active) >> end_active = start_active; >> diff --git a/src/devices/alsa.cpp b/src/devices/alsa.cpp >> index 4f5d3f9..a67780c 100644 >> --- a/src/devices/alsa.cpp >> +++ b/src/devices/alsa.cpp >> @@ -104,7 +104,7 @@ void alsa::start_measurement(void) >> file.close(); >> } >> catch (std::ios_base::failure &c) { >> - fprintf(stderr, "%s\n", c.what()); >> + fprintf(stderr, "Failed to start measurement for alsa >> device\n"); >> } >> } >> @@ -130,7 +130,7 @@ void alsa::end_measurement(void) >> file.close(); >> } >> catch (std::ios_base::failure &c) { >> - fprintf(stderr, "%s\n", c.what()); >> + fprintf(stderr, "Failed to end measurement for alsa >> device\n"); >> } >> p = (end_active - start_active) / (0.001 + end_active + >> end_inactive - start_active - start_inactive) * 100.0; >> diff --git a/src/devices/network.cpp b/src/devices/network.cpp >> index b8a5c9c..ed9d7aa 100644 >> --- a/src/devices/network.cpp >> +++ b/src/devices/network.cpp >> @@ -55,22 +55,6 @@ extern "C" { >> static map<string, class network *> nics; >> -#ifdef DISABLE_TRYCATCH >> - >> -static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep, >> - __u32 speed) >> -{ >> - >> - ep->speed = (__u16)speed; >> - ep->speed_hi = (__u16)(speed >> 16); >> -} >> - >> -static inline __u32 ethtool_cmd_speed(struct ethtool_cmd *ep) >> -{ >> - return (ep->speed_hi << 16) | ep->speed; >> -} >> - >> -#endif >> static void do_proc_net_dev(void) >> { >> diff --git a/src/main.cpp b/src/main.cpp >> index 1815075..dc49dba 100644 >> --- a/src/main.cpp >> +++ b/src/main.cpp >> @@ -42,7 +42,9 @@ >> #include "perf/perf.h" >> #include "perf/perf_bundle.h" >> #include "lib.h" >> +#ifdef HAVE_CONFIG_H >> #include "../config.h" >> +#endif >> #include "devices/device.h" > >
On 9/25/2012 7:34 AM, Rajagopal Venkat wrote: >> On 09/24/2012 06:28 AM, Rajagopal Venkat wrote: >>> >>> This patch adds following minor changes to prepare powertop >>> to support Android platform. >>> >>> - Add missing HAVE_CONFIG_H conditional check. >>> - remove un-used ethtool_cmd_speed_set and ethtool_cmd_speed >>> functions. >>> - Minimize dependency on exception handling in catch blocks. >>> >>> These changes will not affect powertop functionality. >>> >>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> >> > As discussed in first patch set, android doesn't support exception > handling. This where is this in your commit log? > is the reason powertop had DISABLE_TRYCATCH conditional macro which is > removed in recent commit. Android being braindead about the C++ language should be it's own patch, not snuck in as part of another change. > > The patch 2/2 adds stubs for exception handling > > #define try if (true) > #define catch(x) if (false) > > With this, fprintf(stderr, "%s\n", c.what()); in catch block throws > undefined reference > to c. So added message instead of c.what(). > > Any better ways of adding stubs are welcome. #define try(x) if (true) { x; #define catch(x) } else ?
On 9/25/2012 7:34 AM, Rajagopal Venkat wrote: >> On 09/24/2012 06:28 AM, Rajagopal Venkat wrote: >>> >>> This patch adds following minor changes to prepare powertop >>> to support Android platform. >>> >>> - Add missing HAVE_CONFIG_H conditional check. >>> - remove un-used ethtool_cmd_speed_set and ethtool_cmd_speed >>> functions. >>> - Minimize dependency on exception handling in catch blocks. >>> >>> These changes will not affect powertop functionality. >>> >>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> >> > As discussed in first patch set, android doesn't support exception > handling. This where is this in your commit log? > is the reason powertop had DISABLE_TRYCATCH conditional macro which is > removed in recent commit. Android being braindead about the C++ language should be it's own patch, not snuck in as part of another change. > > The patch 2/2 adds stubs for exception handling > > #define try if (true) > #define catch(x) if (false) > > With this, fprintf(stderr, "%s\n", c.what()); in catch block throws > undefined reference > to c. So added message instead of c.what(). > > Any better ways of adding stubs are welcome. #define try(x) if (true) { x; #define catch(x) } else ?
On 09/24/2012 10:34 PM, Rajagopal Venkat wrote: > On 24 September 2012 21:27, Chris Ferron <chris.e.ferron@linux.intel.com> wrote: >> On 09/24/2012 06:28 AM, Rajagopal Venkat wrote: >>> This patch adds following minor changes to prepare powertop >>> to support Android platform. >>> >>> - Add missing HAVE_CONFIG_H conditional check. >>> - remove un-used ethtool_cmd_speed_set and ethtool_cmd_speed >>> functions. >>> - Minimize dependency on exception handling in catch blocks. >>> >>> These changes will not affect powertop functionality. >>> >>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> >>> --- >>> src/devices/ahci.cpp | 4 ++-- >>> src/devices/alsa.cpp | 4 ++-- >>> src/devices/network.cpp | 16 ---------------- >>> src/main.cpp | 2 ++ >>> 4 files changed, 6 insertions(+), 20 deletions(-) >>> >>> diff --git a/src/devices/ahci.cpp b/src/devices/ahci.cpp >>> index 1fe39c7..67ce06e 100644 >>> --- a/src/devices/ahci.cpp >>> +++ b/src/devices/ahci.cpp >>> @@ -170,7 +170,7 @@ void ahci::start_measurement(void) >>> file.close(); >>> } >>> catch (std::ios_base::failure &c) { >>> - fprintf(stderr, "%s\n", c.what()); >>> + fprintf(stderr, "Failed to start measurement for ahci >>> device\n"); >> adding addition message here is acceptable, but eliminating the information >> from the catches error is not. >> > As discussed in first patch set, android doesn't support exception > handling. This > is the reason powertop had DISABLE_TRYCATCH conditional macro which is > removed in recent commit. > > The patch 2/2 adds stubs for exception handling > > #define try if (true) > #define catch(x) if (false) > > With this, fprintf(stderr, "%s\n", c.what()); in catch block throws > undefined reference > to c. So added message instead of c.what(). > > Any better ways of adding stubs are welcome. So it is my opinion that this is a bug for deficiency's in Android, and one that is not appropriate to work around in this core tool. Any patch/fix/workaround should be submitted to the people maintaining Android. The reason the DISABLE_TRYCATCH was removed was to eliminate any non generic need for defines within the tool. We are actively trying to eliminate and discouraging distribution specific code within the core PowerTOP source. There is some flex ability for example your stub header for android. My current suggestion is to re-submit your patches minus the Android workarounds for C++ try-catches, and then work this issues with the fine people at google. PowerTOP focus should be on platforms, architectures, and the Linux kernel, not distributions. -C >>> } >>> } >>> @@ -203,7 +203,7 @@ void ahci::end_measurement(void) >>> file.close(); >>> } >>> catch (std::ios_base::failure &c) { >>> - fprintf(stderr, "%s\n", c.what()); >>> + fprintf(stderr, "Failed to end measurement for ahci >>> device\n"); >>> } >>> if (end_active < start_active) >>> end_active = start_active; >>> diff --git a/src/devices/alsa.cpp b/src/devices/alsa.cpp >>> index 4f5d3f9..a67780c 100644 >>> --- a/src/devices/alsa.cpp >>> +++ b/src/devices/alsa.cpp >>> @@ -104,7 +104,7 @@ void alsa::start_measurement(void) >>> file.close(); >>> } >>> catch (std::ios_base::failure &c) { >>> - fprintf(stderr, "%s\n", c.what()); >>> + fprintf(stderr, "Failed to start measurement for alsa >>> device\n"); >>> } >>> } >>> @@ -130,7 +130,7 @@ void alsa::end_measurement(void) >>> file.close(); >>> } >>> catch (std::ios_base::failure &c) { >>> - fprintf(stderr, "%s\n", c.what()); >>> + fprintf(stderr, "Failed to end measurement for alsa >>> device\n"); >>> } >>> p = (end_active - start_active) / (0.001 + end_active + >>> end_inactive - start_active - start_inactive) * 100.0; >>> diff --git a/src/devices/network.cpp b/src/devices/network.cpp >>> index b8a5c9c..ed9d7aa 100644 >>> --- a/src/devices/network.cpp >>> +++ b/src/devices/network.cpp >>> @@ -55,22 +55,6 @@ extern "C" { >>> static map<string, class network *> nics; >>> -#ifdef DISABLE_TRYCATCH >>> - >>> -static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep, >>> - __u32 speed) >>> -{ >>> - >>> - ep->speed = (__u16)speed; >>> - ep->speed_hi = (__u16)(speed >> 16); >>> -} >>> - >>> -static inline __u32 ethtool_cmd_speed(struct ethtool_cmd *ep) >>> -{ >>> - return (ep->speed_hi << 16) | ep->speed; >>> -} >>> - >>> -#endif >>> static void do_proc_net_dev(void) >>> { >>> diff --git a/src/main.cpp b/src/main.cpp >>> index 1815075..dc49dba 100644 >>> --- a/src/main.cpp >>> +++ b/src/main.cpp >>> @@ -42,7 +42,9 @@ >>> #include "perf/perf.h" >>> #include "perf/perf_bundle.h" >>> #include "lib.h" >>> +#ifdef HAVE_CONFIG_H >>> #include "../config.h" >>> +#endif >>> #include "devices/device.h" >> > >
diff --git a/src/devices/ahci.cpp b/src/devices/ahci.cpp index 1fe39c7..67ce06e 100644 --- a/src/devices/ahci.cpp +++ b/src/devices/ahci.cpp @@ -170,7 +170,7 @@ void ahci::start_measurement(void) file.close(); } catch (std::ios_base::failure &c) { - fprintf(stderr, "%s\n", c.what()); + fprintf(stderr, "Failed to start measurement for ahci device\n"); } } @@ -203,7 +203,7 @@ void ahci::end_measurement(void) file.close(); } catch (std::ios_base::failure &c) { - fprintf(stderr, "%s\n", c.what()); + fprintf(stderr, "Failed to end measurement for ahci device\n"); } if (end_active < start_active) end_active = start_active; diff --git a/src/devices/alsa.cpp b/src/devices/alsa.cpp index 4f5d3f9..a67780c 100644 --- a/src/devices/alsa.cpp +++ b/src/devices/alsa.cpp @@ -104,7 +104,7 @@ void alsa::start_measurement(void) file.close(); } catch (std::ios_base::failure &c) { - fprintf(stderr, "%s\n", c.what()); + fprintf(stderr, "Failed to start measurement for alsa device\n"); } } @@ -130,7 +130,7 @@ void alsa::end_measurement(void) file.close(); } catch (std::ios_base::failure &c) { - fprintf(stderr, "%s\n", c.what()); + fprintf(stderr, "Failed to end measurement for alsa device\n"); } p = (end_active - start_active) / (0.001 + end_active + end_inactive - start_active - start_inactive) * 100.0; diff --git a/src/devices/network.cpp b/src/devices/network.cpp index b8a5c9c..ed9d7aa 100644 --- a/src/devices/network.cpp +++ b/src/devices/network.cpp @@ -55,22 +55,6 @@ extern "C" { static map<string, class network *> nics; -#ifdef DISABLE_TRYCATCH - -static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep, - __u32 speed) -{ - - ep->speed = (__u16)speed; - ep->speed_hi = (__u16)(speed >> 16); -} - -static inline __u32 ethtool_cmd_speed(struct ethtool_cmd *ep) -{ - return (ep->speed_hi << 16) | ep->speed; -} - -#endif static void do_proc_net_dev(void) { diff --git a/src/main.cpp b/src/main.cpp index 1815075..dc49dba 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -42,7 +42,9 @@ #include "perf/perf.h" #include "perf/perf_bundle.h" #include "lib.h" +#ifdef HAVE_CONFIG_H #include "../config.h" +#endif #include "devices/device.h"
This patch adds following minor changes to prepare powertop to support Android platform. - Add missing HAVE_CONFIG_H conditional check. - remove un-used ethtool_cmd_speed_set and ethtool_cmd_speed functions. - Minimize dependency on exception handling in catch blocks. These changes will not affect powertop functionality. Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> --- src/devices/ahci.cpp | 4 ++-- src/devices/alsa.cpp | 4 ++-- src/devices/network.cpp | 16 ---------------- src/main.cpp | 2 ++ 4 files changed, 6 insertions(+), 20 deletions(-)