7:27 a.m.
Hello,
The following 15 patches are re-submission. Reply indicators are related
to feedback from:
https://lists.01.org/pipermail/powertop/2014-August/001495.html
0- When creating your patches modify the patch set subject using:
git
format-patch --cover-letter --subject-prefix="PATCH Vx"... Where x is
the next version for your patches. For your next version use v3.
This very useful for sorting purposes on my inbox.
That makes sense. Done.
1- Every time you make changes to your patches you would need to
send
the entire set of patches with an updated version. This link
explains patch best practices that we follow on PowerTOP and across
the Linux kernel community:
http://kernelnewbies.org/OPWfirstpatch#head-067c0e555973ae364180b2e3fbe90...
Are you sure kernel newbies page is telling to resubmit long patch sets
when there is only modest change is some in middle? IMHO resubmitting
individual patches that has changed when combined with a git remote
repository containing the full patch set is good way to do keep noise
level low.
But then again all projects has some habits that I don't understand/agree
with which shouldn't really be a problem. I will keep on resubmitting
patch sets when requested to do so (and protest a bit by having the git
remote available ;-)
3- It is a good practice to add the names of the people who reviewed
your patches, at the commit paragraph Reviewed_by:[the name of the
person].
Done.
Here is my review to the 14 patches you sent on v2 of your code:
P1, P2, P4, P5, P6, P10, P11, P12, P13, and P14:
-Look good.
Great.
P3:
- Change the subject line to something more descriptive. Add a
description of why are you making this change, and how it benefits
PowerTOP. The subject lines and descriptions are very useful when
bisecting the tree for debugging purposes.
Done: add src/cpu/rapl/.deps to .gitignore
P7:
- Please add a description to this patch. (Same reasoning as on P3)
See the patch 0007.
P8:
- I have a problem naming -h --html and -u --help. Please change -h
to
--help, and use another letter for --html for instance: r as of
report would be more fitting. New users will want to do powertop -h
and see the help not the html report and get totally lost. It is
just matter of reusing the already established usability metaphors.
I did change -h to --help, which is ABI change to undocumented interface,
which may or may not better than having unconventional short option
assignment. BTW I think it would be good to add this change to release
notes, just that no-one can later tell breaking changes happen without
noise.
P9:
- Please abbreviate the subject and add a description to this patch.
Done.
- To make this a feature-complete Sergey's feedback should be
implemented: "...to extend ui_notify_user() so it can cover both
cases: initialised ncurses -- calling mvprintw(), and uninitialised
ncurses -- printf (to stdout). this will change a bit 'powertop
--auto-tune' behaviour, since by default user will now see a 'list'
of commands executed by powertop, but it's really easy to silent
powertop via stdout redirection".
As Sergey mentioned the mvprintw() avoidance is not needed. But since I
figured out how to make both cases, ncurses and console, work without
pain a patch 0014 was added to the change set. At the moment one can say
usefulness of the change is close to zero, as the ui_notify_user() is not
reached in case of --auto-tune, but perhaps that changes in future. And
then it's nice to have a facility in place that just works.
The following changes since commit cdff23d6e40e31a68286a51e3c94c8a901a74091:
powertop: remove temporary file (2014-08-22 09:18:34 -0700)
are available in the git repository at:
git://github.com/kerolasa/powertop.git sami
for you to fetch changes up to 295c86c4f5fa22024792e8e850f303eec7f4b6a3:
allow none-ncurses messaging with ui_notify_user() (2014-09-07 21:52:48 +0100)
----------------------------------------------------------------
Sami Kerola (15):
configure: use vertical lists
configure: prefer AS_IF macro rather than shell if statement
add src/cpu/rapl/.deps to .gitignore
clean up Makefile.am files
fix clang++ compilation errors
remove unnecessary assignments
remove unnecessary code reported by cppcheck
add short options to usage, and reorder options in code
do not use ncurses with --auto-tune
improve manual groff syntax
fix couple typos
wrap lines in README.traceevent
clean up .gitignore file
add --auto-tune and --workload to manual page
allow none-ncurses messaging with ui_notify_user()
.gitignore | 110 ++++++++++-------------
Makefile.am | 14 ++-
README.traceevent | 21 +++--
configure.ac | 102 ++++++++++++++++-----
doc/powertop.8 | 149 ++++++++++++++++---------------
src/Makefile.am | 193 +++++++++++++++++++++++++++++++---------
src/cpu/cpu.cpp | 18 ++--
src/cpu/rapl/rapl_interface.cpp | 4 +-
src/devices/ahci.cpp | 3 +-
src/devices/alsa.cpp | 1 -
src/devices/device.cpp | 6 +-
src/devlist.cpp | 3 +-
src/display.cpp | 9 +-
src/lib.cpp | 19 ++--
src/lib.h | 4 +-
src/main.cpp | 181 +++++++++++++++++++------------------
src/process/do_process.cpp | 11 ++-
src/report/report.cpp | 3 +-
src/tuning/nl80211.h | 4 +-
src/tuning/runtime.cpp | 4 -
src/tuning/tuning.cpp | 11 +--
21 files changed, 533 insertions(+), 337 deletions(-)
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 01/15] configure: use vertical lists
Horizontal lists has downside of being managed as a single entry in
version control, which makes commits that add and remove items difficult
to follow. With vertical lists each entry is managed separately, which
is much nicer.
Additionally add project URL to autoconfig init, and pretty print macro
expressions to have indentation, and bracing that follows more or less in
K&R style.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
configure.ac | 82 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 67 insertions(+), 15 deletions(-)
diff --git a/configure.ac b/configure.ac
index 0b02ad2..b41cfb3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,15 +2,21 @@
# Process this file with autoconf to produce a configure script.
AC_PREREQ([2.68])
-AC_INIT([powertop], [2.6.1], [powertop(a)lists.01.org])
-AM_INIT_AUTOMAKE([-Wall foreign ])
+AC_INIT([powertop], [2.6.1], [powertop(a)lists.01.org], [], [https://01.org/powertop])
+AM_INIT_AUTOMAKE([-Wall foreign])
AC_LANG([C++])
-AC_CONFIG_FILES([Makefile src/Makefile traceevent/Makefile po/Makefile.in doc/Makefile])
+AC_CONFIG_FILES([
+ Makefile
+ src/Makefile
+ traceevent/Makefile
+ po/Makefile.in
+ doc/Makefile
+])
AC_CONFIG_SRCDIR([src/main.cpp])
AC_CONFIG_MACRO_DIR([m4])
AC_CONFIG_HEADERS([config.h])
GETTEXT_PACKAGE=powertop
-AC_SUBST(GETTEXT_PACKAGE)
+AC_SUBST([GETTEXT_PACKAGE])
AM_SILENT_RULES([yes])
AM_GNU_GETTEXT([external])
AM_GNU_GETTEXT_VERSION([0.18])
@@ -26,7 +32,23 @@ AM_PROG_CC_C_O
# Checks for libraries.
# Checks for header files.
-AC_CHECK_HEADERS([fcntl.h libintl.h limits.h locale.h malloc.h stdint.h stdlib.h string.h
sys/ioctl.h sys/socket.h sys/statfs.h sys/time.h termios.h unistd.h ncurses.h])
+AC_CHECK_HEADERS([ \
+ fcntl.h \
+ libintl.h \
+ limits.h \
+ locale.h \
+ malloc.h \
+ ncurses.h \
+ stdint.h \
+ stdlib.h \
+ string.h \
+ sys/ioctl.h \
+ sys/socket.h \
+ sys/statfs.h \
+ sys/time.h \
+ termios.h \
+ unistd.h \
+])
# Checks for typedefs, structures, and compiler characteristics.
AC_HEADER_STDBOOL
@@ -44,20 +66,46 @@ AC_FUNC_MALLOC
AC_FUNC_MMAP
AC_FUNC_REALLOC
AC_FUNC_STRTOD
-AC_CHECK_FUNCS([fdatasync getpagesize gettimeofday memmove memset mkdir munmap pow
realpath regcomp select setlocale socket sqrt strcasecmp strchr strdup strerror
strncasecmp strstr strtoul strtoull])
+AC_CHECK_FUNCS([ \
+ fdatasync \
+ getpagesize \
+ gettimeofday \
+ memmove \
+ memset \
+ mkdir \
+ munmap \
+ pow \
+ realpath \
+ regcomp \
+ select \
+ setlocale \
+ socket \
+ sqrt \
+ strcasecmp \
+ strchr \
+ strdup \
+ strerror \
+ strncasecmp \
+ strstr \
+ strtoul \
+ strtoull \
+])
-PKG_CHECK_MODULES([NCURSES], [ncursesw ncurses], [LIBS="$LIBS $ncurses_LIBS"],
- AC_SEARCH_LIBS([delwin], [ncursesw ncurses], [], AC_MSG_ERROR([ncurses is required but
was not found]), []))
+PKG_CHECK_MODULES([NCURSES], [ncursesw ncurses], [LIBS="$LIBS $ncurses_LIBS"],
[
+ AC_SEARCH_LIBS([delwin], [ncursesw ncurses], [], [
+ AC_MSG_ERROR([ncurses is required but was not found])
+ ], [])
+])
has_libpci=0
-PKG_CHECK_MODULES([PCIUTILS], [libpci],[has_libpci=1],[
- AC_SEARCH_LIBS([pci_get_dev], [pci],[has_libpci=1], [has_libpci=0] )])
+PKG_CHECK_MODULES([PCIUTILS], [libpci],[has_libpci=1], [
+ AC_SEARCH_LIBS([pci_get_dev], [pci],[has_libpci=1], [has_libpci=0])
+])
has_libnl_ver=0
-# libnl-2 provides only libnl-2.0.pc file, so we check for separate libnl-genl-3.0.pc
-# pkg-config file just for libnl-3.0 case.
-#
+dnl libnl-2 provides only libnl-2.0.pc file, so we check for separate
+dnl libnl-genl-3.0.pc pkg-config file just for libnl-3.0 case.
PKG_CHECK_MODULES([LIBNL], [libnl-3.0 >= 3.0 libnl-genl-3.0 >= 3.0],
[has_libnl_ver=3], [
PKG_CHECK_MODULES([LIBNL], [libnl-2.0 >= 2.0], [has_libnl_ver=2], [
PKG_CHECK_MODULES([LIBNL], [libnl-1], [has_libnl_ver=1], [has_libnl_ver=0])])])
@@ -78,7 +126,11 @@ if (test "$has_libpci" -eq 0); then
])
fi
-AC_SEARCH_LIBS([pthread_create], [pthread], [], AC_MSG_ERROR([libpthread is required but
was not found]), [])
-AC_SEARCH_LIBS([inet_aton], [resolv], [], AC_MSG_ERROR([libresolv is required but was not
found]), [])
+AC_SEARCH_LIBS([pthread_create], [pthread], [], [
+ AC_MSG_ERROR([libpthread is required but was not found])
+], [])
+AC_SEARCH_LIBS([inet_aton], [resolv], [], [
+ AC_MSG_ERROR([libresolv is required but was not found])
+], [])
AC_OUTPUT
--
2.1.0
10:20 a.m.
New subject: [Powertop] [PATCH v3 01/15] configure: use vertical lists
This series was merged 29 Sept, I know, but I had skin in this series,
so here's the (belated) review...
LGTM.
Reviewed-By: Joe Konno <joe.konno(a)intel.com>
On 09/07/2014 02:27 PM, Sami Kerola wrote:
Horizontal lists has downside of being managed as a single entry in
version control, which makes commits that add and remove items difficult
to follow. With vertical lists each entry is managed separately, which
is much nicer.
Additionally add project URL to autoconfig init, and pretty print macro
expressions to have indentation, and bracing that follows more or less in
K&R style.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
configure.ac | 82 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 67 insertions(+), 15 deletions(-)
diff --git a/configure.ac b/configure.ac
index 0b02ad2..b41cfb3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,15 +2,21 @@
# Process this file with autoconf to produce a configure script.
AC_PREREQ([2.68])
-AC_INIT([powertop], [2.6.1], [powertop(a)lists.01.org])
-AM_INIT_AUTOMAKE([-Wall foreign ])
+AC_INIT([powertop], [2.6.1], [powertop(a)lists.01.org], [], [https://01.org/powertop])
+AM_INIT_AUTOMAKE([-Wall foreign])
AC_LANG([C++])
-AC_CONFIG_FILES([Makefile src/Makefile traceevent/Makefile po/Makefile.in
doc/Makefile])
+AC_CONFIG_FILES([
+ Makefile
+ src/Makefile
+ traceevent/Makefile
+ po/Makefile.in
+ doc/Makefile
+])
AC_CONFIG_SRCDIR([src/main.cpp])
AC_CONFIG_MACRO_DIR([m4])
AC_CONFIG_HEADERS([config.h])
GETTEXT_PACKAGE=powertop
-AC_SUBST(GETTEXT_PACKAGE)
+AC_SUBST([GETTEXT_PACKAGE])
AM_SILENT_RULES([yes])
AM_GNU_GETTEXT([external])
AM_GNU_GETTEXT_VERSION([0.18])
@@ -26,7 +32,23 @@ AM_PROG_CC_C_O
# Checks for libraries.
# Checks for header files.
-AC_CHECK_HEADERS([fcntl.h libintl.h limits.h locale.h malloc.h stdint.h stdlib.h
string.h sys/ioctl.h sys/socket.h sys/statfs.h sys/time.h termios.h unistd.h ncurses.h])
+AC_CHECK_HEADERS([ \
+ fcntl.h \
+ libintl.h \
+ limits.h \
+ locale.h \
+ malloc.h \
+ ncurses.h \
+ stdint.h \
+ stdlib.h \
+ string.h \
+ sys/ioctl.h \
+ sys/socket.h \
+ sys/statfs.h \
+ sys/time.h \
+ termios.h \
+ unistd.h \
+])
# Checks for typedefs, structures, and compiler characteristics.
AC_HEADER_STDBOOL
@@ -44,20 +66,46 @@ AC_FUNC_MALLOC
AC_FUNC_MMAP
AC_FUNC_REALLOC
AC_FUNC_STRTOD
-AC_CHECK_FUNCS([fdatasync getpagesize gettimeofday memmove memset mkdir munmap pow
realpath regcomp select setlocale socket sqrt strcasecmp strchr strdup strerror
strncasecmp strstr strtoul strtoull])
+AC_CHECK_FUNCS([ \
+ fdatasync \
+ getpagesize \
+ gettimeofday \
+ memmove \
+ memset \
+ mkdir \
+ munmap \
+ pow \
+ realpath \
+ regcomp \
+ select \
+ setlocale \
+ socket \
+ sqrt \
+ strcasecmp \
+ strchr \
+ strdup \
+ strerror \
+ strncasecmp \
+ strstr \
+ strtoul \
+ strtoull \
+])
-PKG_CHECK_MODULES([NCURSES], [ncursesw ncurses], [LIBS="$LIBS
$ncurses_LIBS"],
- AC_SEARCH_LIBS([delwin], [ncursesw ncurses], [], AC_MSG_ERROR([ncurses is required but
was not found]), []))
+PKG_CHECK_MODULES([NCURSES], [ncursesw ncurses], [LIBS="$LIBS $ncurses_LIBS"],
[
+ AC_SEARCH_LIBS([delwin], [ncursesw ncurses], [], [
+ AC_MSG_ERROR([ncurses is required but was not found])
+ ], [])
+])
has_libpci=0
-PKG_CHECK_MODULES([PCIUTILS], [libpci],[has_libpci=1],[
- AC_SEARCH_LIBS([pci_get_dev], [pci],[has_libpci=1], [has_libpci=0] )])
+PKG_CHECK_MODULES([PCIUTILS], [libpci],[has_libpci=1], [
+ AC_SEARCH_LIBS([pci_get_dev], [pci],[has_libpci=1], [has_libpci=0])
+])
has_libnl_ver=0
-# libnl-2 provides only libnl-2.0.pc file, so we check for separate libnl-genl-3.0.pc
-# pkg-config file just for libnl-3.0 case.
-#
+dnl libnl-2 provides only libnl-2.0.pc file, so we check for separate
+dnl libnl-genl-3.0.pc pkg-config file just for libnl-3.0 case.
PKG_CHECK_MODULES([LIBNL], [libnl-3.0 >= 3.0 libnl-genl-3.0 >= 3.0],
[has_libnl_ver=3], [
PKG_CHECK_MODULES([LIBNL], [libnl-2.0 >= 2.0], [has_libnl_ver=2], [
PKG_CHECK_MODULES([LIBNL], [libnl-1], [has_libnl_ver=1], [has_libnl_ver=0])])])
@@ -78,7 +126,11 @@ if (test "$has_libpci" -eq 0); then
])
fi
-AC_SEARCH_LIBS([pthread_create], [pthread], [], AC_MSG_ERROR([libpthread is required but
was not found]), [])
-AC_SEARCH_LIBS([inet_aton], [resolv], [], AC_MSG_ERROR([libresolv is required but was
not found]), [])
+AC_SEARCH_LIBS([pthread_create], [pthread], [], [
+ AC_MSG_ERROR([libpthread is required but was not found])
+], [])
+AC_SEARCH_LIBS([inet_aton], [resolv], [], [
+ AC_MSG_ERROR([libresolv is required but was not found])
+], [])
AC_OUTPUT
7:27 a.m.
New subject: [Powertop] [PATCH v3 02/15] configure: prefer AS_IF macro rather than shell if statement
Jumping in between shell and m4 syntaxes makes reading the code more
difficult, so stick to m4 when ever possible.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
configure.ac | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/configure.ac b/configure.ac
index b41cfb3..4991d97 100644
--- a/configure.ac
+++ b/configure.ac
@@ -108,23 +108,25 @@ dnl libnl-2 provides only libnl-2.0.pc file, so we check for
separate
dnl libnl-genl-3.0.pc pkg-config file just for libnl-3.0 case.
PKG_CHECK_MODULES([LIBNL], [libnl-3.0 >= 3.0 libnl-genl-3.0 >= 3.0],
[has_libnl_ver=3], [
PKG_CHECK_MODULES([LIBNL], [libnl-2.0 >= 2.0], [has_libnl_ver=2], [
- PKG_CHECK_MODULES([LIBNL], [libnl-1], [has_libnl_ver=1], [has_libnl_ver=0])])])
-if (test "$has_libnl_ver" -eq 0); then
- AC_MSG_ERROR(libnl and libnl-genl are required but were not found)
-fi
-if (test "$has_libnl_ver" -gt 1); then
+ PKG_CHECK_MODULES([LIBNL], [libnl-1], [has_libnl_ver=1], [has_libnl_ver=0])
+ ])
+])
+AS_IF([test "$has_libnl_ver" -eq 0], [
+ AC_MSG_ERROR([libnl and libnl-genl are required but were not found])
+])
+AS_IF([test "$has_libnl_ver" -gt 1], [
AC_DEFINE([HAVE_LIBNL20], [1], [Define if you have libnl-2.0 or higher])
-fi
+])
-if (test "$has_libpci" -eq 0); then
- AC_DEFINE([HAVE_NO_PCI],[1],[Define if pci is not supported])
+AS_IF([test "$has_libpci" -eq 0], [
+ AC_DEFINE([HAVE_NO_PCI], [1], [Define if pci is not supported])
AC_MSG_WARN([
************* LIBPCI SUPPORT NOT CONFIGURED**************
If you need or want pci support, please install libpci
and re-configure PowerTOP.
*********************************************************
])
-fi
+])
AC_SEARCH_LIBS([pthread_create], [pthread], [], [
AC_MSG_ERROR([libpthread is required but was not found])
--
2.1.0
10:29 a.m.
New subject: [Powertop] [PATCH v3 02/15] configure: prefer AS_IF macro rather than shell if statement
LGTM.
Reviewed-By: Joe Konno <joe.konno(a)intel.com>
On 09/07/2014 02:27 PM, Sami Kerola wrote:
Jumping in between shell and m4 syntaxes makes reading the code more
difficult, so stick to m4 when ever possible.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
configure.ac | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/configure.ac b/configure.ac
index b41cfb3..4991d97 100644
--- a/configure.ac
+++ b/configure.ac
@@ -108,23 +108,25 @@ dnl libnl-2 provides only libnl-2.0.pc file, so we check for
separate
dnl libnl-genl-3.0.pc pkg-config file just for libnl-3.0 case.
PKG_CHECK_MODULES([LIBNL], [libnl-3.0 >= 3.0 libnl-genl-3.0 >= 3.0],
[has_libnl_ver=3], [
PKG_CHECK_MODULES([LIBNL], [libnl-2.0 >= 2.0], [has_libnl_ver=2], [
- PKG_CHECK_MODULES([LIBNL], [libnl-1], [has_libnl_ver=1], [has_libnl_ver=0])])])
-if (test "$has_libnl_ver" -eq 0); then
- AC_MSG_ERROR(libnl and libnl-genl are required but were not found)
-fi
-if (test "$has_libnl_ver" -gt 1); then
+ PKG_CHECK_MODULES([LIBNL], [libnl-1], [has_libnl_ver=1], [has_libnl_ver=0])
+ ])
+])
+AS_IF([test "$has_libnl_ver" -eq 0], [
+ AC_MSG_ERROR([libnl and libnl-genl are required but were not found])
+])
+AS_IF([test "$has_libnl_ver" -gt 1], [
AC_DEFINE([HAVE_LIBNL20], [1], [Define if you have libnl-2.0 or higher])
-fi
+])
-if (test "$has_libpci" -eq 0); then
- AC_DEFINE([HAVE_NO_PCI],[1],[Define if pci is not supported])
+AS_IF([test "$has_libpci" -eq 0], [
+ AC_DEFINE([HAVE_NO_PCI], [1], [Define if pci is not supported])
AC_MSG_WARN([
************* LIBPCI SUPPORT NOT CONFIGURED**************
If you need or want pci support, please install libpci
and re-configure PowerTOP.
*********************************************************
])
-fi
+])
AC_SEARCH_LIBS([pthread_create], [pthread], [], [
AC_MSG_ERROR([libpthread is required but was not found])
7:27 a.m.
New subject: [Powertop] [PATCH v3 03/15] add src/cpu/rapl/.deps to .gitignore
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
.gitignore | 1 +
1 file changed, 1 insertion(+)
diff --git a/.gitignore b/.gitignore
index 35d41c2..d00973f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -44,6 +44,7 @@ src/.deps/
src/Makefile
src/calibrate/.deps/
src/cpu/.deps/
+src/cpu/rapl/.deps
src/devices/.deps/
src/measurement/.deps/
src/parameters/.deps/
--
2.1.0
10:32 a.m.
New subject: [Powertop] [PATCH v3 03/15] add src/cpu/rapl/.deps to .gitignore
LGTM.
Reviewed-By: Joe Konno <joe.konno(a)intel.com>
On 09/07/2014 02:27 PM, Sami Kerola wrote:
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
.gitignore | 1 +
1 file changed, 1 insertion(+)
diff --git a/.gitignore b/.gitignore
index 35d41c2..d00973f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -44,6 +44,7 @@ src/.deps/
src/Makefile
src/calibrate/.deps/
src/cpu/.deps/
+src/cpu/rapl/.deps
src/devices/.deps/
src/measurement/.deps/
src/parameters/.deps/
7:27 a.m.
New subject: [Powertop] [PATCH v3 04/15] clean up Makefile.am files
Use vertical rather than horizontal lists, and get rid of deprecated
ACLOCAL_AMFLAGS instruction. The src/Makefile.am source file list also
needed sorting, as the items order seemed more or less random.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
Makefile.am | 14 +++-
src/Makefile.am | 193 +++++++++++++++++++++++++++++++++++++++++++-------------
2 files changed, 161 insertions(+), 46 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index f5e466b..c8d0ae2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,14 @@
AUTOMAKE_OPTIONS = subdir-objects
-ACLOCAL_AMFLAGS = -I m4
-SUBDIRS = traceevent src po doc
+SUBDIRS = \
+ traceevent \
+ src \
+ po \
+ doc
-EXTRA_DIST = README TODO Android.mk COPYING autogen.sh
+EXTRA_DIST = \
+ README \
+ TODO \
+ Android.mk \
+ COPYING \
+ autogen.sh
diff --git a/src/Makefile.am b/src/Makefile.am
index 6886388..311b75e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1,54 +1,161 @@
AUTOMAKE_OPTIONS = subdir-objects
-ACLOCAL_AMFLAGS = -I ../m4
sbin_PROGRAMS = powertop
nodist_powertop_SOURCES = css.h
-powertop_SOURCES = parameters/persistent.cpp parameters/learn.cpp
parameters/parameters.cpp \
- parameters/parameters.h display.cpp cpu/cpu_core.cpp cpu/cpudevice.cpp \
- cpu/cpu.cpp cpu/cpu.h cpu/cpu_linux.cpp cpu/abstract_cpu.cpp cpu/intel_cpus.h \
- cpu/cpu_package.cpp cpu/cpudevice.h cpu/intel_cpus.cpp devlist.h devlist.cpp \
- lib.cpp report/report.cpp process/process.cpp process/interrupt.h process/timer.h \
- process/processdevice.cpp process/powerconsumer.cpp process/work.h \
- process/timer.cpp process/process.h process/work.cpp process/do_process.cpp \
- process/processdevice.h process/interrupt.cpp process/powerconsumer.h report/report.h
\
- perf/perf.cpp perf/perf_event.h perf/perf.h perf/perf_bundle.h perf/perf_bundle.cpp \
- display.h devices/rfkill.h devices/i915-gpu.cpp devices/device.cpp devices/alsa.cpp \
- devices/runtime_pm.cpp devices/thinkpad-fan.cpp devices/usb.cpp devices/ahci.cpp \
- devices/thinkpad-light.cpp devices/ahci.h devices/i915-gpu.h devices/network.h \
- devices/rfkill.cpp devices/alsa.h devices/thinkpad-fan.h devices/device.h \
- devices/usb.h devices/backlight.h devices/backlight.cpp devices/runtime_pm.h \
- devices/thinkpad-light.h devices/network.cpp lib.h \
- tuning/bluetooth.cpp tuning/tuning.cpp tuning/tuningusb.cpp \
- tuning/ethernet.cpp tuning/bluetooth.h tuning/tuning.h tuning/ethernet.h \
- tuning/tunable.cpp tuning/nl80211.h tuning/iw.c tuning/wifi.cpp tuning/tuningsysfs.h \
- tuning/tuningsysfs.cpp tuning/wifi.h tuning/runtime.cpp tuning/tunable.h \
- tuning/runtime.h tuning/tuningusb.h tuning/iw.h calibrate/calibrate.cpp \
- calibrate/calibrate.h measurement/measurement.cpp \
- measurement/measurement.h measurement/acpi.cpp measurement/sysfs.h
measurement/sysfs.cpp \
- measurement/acpi.h measurement/extech.cpp measurement/extech.h \
- report/report-maker.cpp report/report-maker.h report/report-formatter.h \
- report/report-formatter-base.cpp report/report-formatter-base.h \
- report/report-formatter-csv.cpp report/report-formatter-csv.h \
- report/report-formatter-html.cpp report/report-formatter-html.h \
- report/report-data-html.cpp report/report-data-html.h \
- main.cpp css.h powertop.css cpu/intel_gpu.cpp \
- cpu/rapl/rapl_interface.cpp cpu/cpu_rapl_device.cpp cpu/rapl/rapl_interface.h\
- cpu/dram_rapl_device.cpp devices/gpu_rapl_device.cpp cpu/cpu_rapl_device.h \
- cpu/dram_rapl_device.h devices/gpu_rapl_device.h
-
-
-powertop_CXXFLAGS = -fno-omit-frame-pointer -fstack-protector -Wall -Wshadow -Wformat
$(NCURSES_CFLAGS) $(PCIUTILS_CFLAGS) $(LIBNL_CFLAGS) $(GLIB2_CFLAGS)
-
-
-powertop_CPPFLAGS = -D_FORTIFY_SOURCE=2 $(NCURSES_CFLAGS) $(PCIUTILS_CFLAGS)
$(LIBNL_CFLAGS) $(GLIB2_CFLAGS) $(LIBZ_CFLAGS) -DLOCALEDIR=\"$(localedir)\"
-
-powertop_LDADD = ../traceevent/libtraceevnet.la
-
-AM_LDFLAGS = $(LIBS) $(NCURSES_LIBS) $(PCIUTILS_LIBS) $(LIBNL_LIBS) $(LIBZ_LIBS)
$(PTHREAD_LIBS) $(RESOLV_LIBS)
+powertop_SOURCES = \
+ css.h \
+ devlist.cpp \
+ devlist.h \
+ display.cpp \
+ display.h \
+ lib.cpp \
+ lib.h \
+ main.cpp \
+ powertop.css \
+ \
+ calibrate/calibrate.cpp \
+ calibrate/calibrate.h \
+ cpu/abstract_cpu.cpp \
+ cpu/cpu.cpp \
+ cpu/cpu.h \
+ cpu/cpu_core.cpp \
+ cpu/cpu_linux.cpp \
+ cpu/cpu_package.cpp \
+ cpu/cpu_rapl_device.cpp \
+ cpu/cpu_rapl_device.h \
+ cpu/cpudevice.cpp \
+ cpu/cpudevice.h \
+ cpu/dram_rapl_device.cpp \
+ cpu/dram_rapl_device.h \
+ cpu/intel_cpus.cpp \
+ cpu/intel_cpus.h \
+ cpu/intel_gpu.cpp \
+ cpu/rapl/rapl_interface.cpp \
+ cpu/rapl/rapl_interface.h \
+ devices/ahci.cpp \
+ devices/ahci.h \
+ devices/alsa.cpp \
+ devices/alsa.h \
+ devices/backlight.cpp \
+ devices/backlight.h \
+ devices/device.cpp \
+ devices/device.h \
+ devices/gpu_rapl_device.cpp \
+ devices/gpu_rapl_device.h \
+ devices/i915-gpu.cpp \
+ devices/i915-gpu.h \
+ devices/network.cpp \
+ devices/network.h \
+ devices/rfkill.cpp \
+ devices/rfkill.h \
+ devices/runtime_pm.cpp \
+ devices/runtime_pm.h \
+ devices/thinkpad-fan.cpp \
+ devices/thinkpad-fan.h \
+ devices/thinkpad-light.cpp \
+ devices/thinkpad-light.h \
+ devices/usb.cpp \
+ devices/usb.h \
+ measurement/acpi.cpp \
+ measurement/acpi.h \
+ measurement/extech.cpp \
+ measurement/extech.h \
+ measurement/measurement.cpp \
+ measurement/measurement.h \
+ measurement/sysfs.cpp \
+ measurement/sysfs.h \
+ parameters/learn.cpp \
+ parameters/parameters.cpp \
+ parameters/parameters.h \
+ parameters/persistent.cpp \
+ perf/perf.cpp \
+ perf/perf.h \
+ perf/perf_bundle.cpp \
+ perf/perf_bundle.h \
+ perf/perf_event.h \
+ process/do_process.cpp \
+ process/interrupt.cpp \
+ process/interrupt.h \
+ process/powerconsumer.cpp \
+ process/powerconsumer.h \
+ process/process.cpp \
+ process/process.h \
+ process/processdevice.cpp \
+ process/processdevice.h \
+ process/timer.cpp \
+ process/timer.h \
+ process/work.cpp \
+ process/work.h \
+ report/report-data-html.cpp \
+ report/report-data-html.h \
+ report/report-formatter-base.cpp \
+ report/report-formatter-base.h \
+ report/report-formatter-csv.cpp \
+ report/report-formatter-csv.h \
+ report/report-formatter-html.cpp \
+ report/report-formatter-html.h \
+ report/report-formatter.h \
+ report/report-maker.cpp \
+ report/report-maker.h \
+ report/report.cpp \
+ report/report.h \
+ tuning/bluetooth.cpp \
+ tuning/bluetooth.h \
+ tuning/ethernet.cpp \
+ tuning/ethernet.h \
+ tuning/iw.c \
+ tuning/iw.h \
+ tuning/nl80211.h \
+ tuning/runtime.cpp \
+ tuning/runtime.h \
+ tuning/tunable.cpp \
+ tuning/tunable.h \
+ tuning/tuning.cpp \
+ tuning/tuning.h \
+ tuning/tuningsysfs.cpp \
+ tuning/tuningsysfs.h \
+ tuning/tuningusb.cpp \
+ tuning/tuningusb.h \
+ tuning/wifi.cpp \
+ tuning/wifi.h
+
+powertop_CXXFLAGS = \
+ -Wall \
+ -Wformat \
+ -Wshadow \
+ -fno-omit-frame-pointer \
+ -fstack-protector \
+ $(GLIB2_CFLAGS) \
+ $(LIBNL_CFLAGS) \
+ $(NCURSES_CFLAGS) \
+ $(PCIUTILS_CFLAGS)
+
+
+powertop_CPPFLAGS = \
+ -DLOCALEDIR=\"$(localedir)\" \
+ -D_FORTIFY_SOURCE=2 \
+ $(GLIB2_CFLAGS) \
+ $(LIBNL_CFLAGS) \
+ $(LIBZ_CFLAGS) \
+ $(NCURSES_CFLAGS) \
+ $(PCIUTILS_CFLAGS)
+
+powertop_LDADD = \
+ ../traceevent/libtraceevnet.la
+
+AM_LDFLAGS = \
+ $(LIBNL_LIBS) \
+ $(LIBS) \
+ $(LIBZ_LIBS) \
+ $(NCURSES_LIBS) \
+ $(PCIUTILS_LIBS) \
+ $(PTHREAD_LIBS) \
+ $(RESOLV_LIBS)
BUILT_SOURCES = css.h
CLEANFILES = css.h
+
css.h: powertop.css
$(SHELL) ${srcdir}/csstoh.sh ${srcdir}/powertop.css css.h
--
2.1.0
10:34 a.m.
New subject: [Powertop] [PATCH v3 04/15] clean up Makefile.am files
Trivial nit, otherwise LGTM.
Reviewed-By: Joe Konno <joe.konno(a)intel.com>
On 09/07/2014 02:27 PM, Sami Kerola wrote:
Use vertical rather than horizontal lists, and get rid of deprecated
ACLOCAL_AMFLAGS instruction. The src/Makefile.am source file list also
needed sorting, as the items order seemed more or less random.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
Makefile.am | 14 +++-
src/Makefile.am | 193 +++++++++++++++++++++++++++++++++++++++++++-------------
2 files changed, 161 insertions(+), 46 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index f5e466b..c8d0ae2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,14 @@
AUTOMAKE_OPTIONS = subdir-objects
-ACLOCAL_AMFLAGS = -I m4
-SUBDIRS = traceevent src po doc
+SUBDIRS = \
+ traceevent \
+ src \
+ po \
+ doc
-EXTRA_DIST = README TODO Android.mk COPYING autogen.sh
+EXTRA_DIST = \
+ README \
+ TODO \
+ Android.mk \
+ COPYING \
+ autogen.sh
diff --git a/src/Makefile.am b/src/Makefile.am
index 6886388..311b75e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1,54 +1,161 @@
AUTOMAKE_OPTIONS = subdir-objects
-ACLOCAL_AMFLAGS = -I ../m4
sbin_PROGRAMS = powertop
nodist_powertop_SOURCES = css.h
-powertop_SOURCES = parameters/persistent.cpp parameters/learn.cpp
parameters/parameters.cpp \
- parameters/parameters.h display.cpp cpu/cpu_core.cpp cpu/cpudevice.cpp \
- cpu/cpu.cpp cpu/cpu.h cpu/cpu_linux.cpp cpu/abstract_cpu.cpp cpu/intel_cpus.h \
- cpu/cpu_package.cpp cpu/cpudevice.h cpu/intel_cpus.cpp devlist.h devlist.cpp \
- lib.cpp report/report.cpp process/process.cpp process/interrupt.h process/timer.h \
- process/processdevice.cpp process/powerconsumer.cpp process/work.h \
- process/timer.cpp process/process.h process/work.cpp process/do_process.cpp \
- process/processdevice.h process/interrupt.cpp process/powerconsumer.h report/report.h
\
- perf/perf.cpp perf/perf_event.h perf/perf.h perf/perf_bundle.h perf/perf_bundle.cpp \
- display.h devices/rfkill.h devices/i915-gpu.cpp devices/device.cpp devices/alsa.cpp \
- devices/runtime_pm.cpp devices/thinkpad-fan.cpp devices/usb.cpp devices/ahci.cpp \
- devices/thinkpad-light.cpp devices/ahci.h devices/i915-gpu.h devices/network.h \
- devices/rfkill.cpp devices/alsa.h devices/thinkpad-fan.h devices/device.h \
- devices/usb.h devices/backlight.h devices/backlight.cpp devices/runtime_pm.h \
- devices/thinkpad-light.h devices/network.cpp lib.h \
- tuning/bluetooth.cpp tuning/tuning.cpp tuning/tuningusb.cpp \
- tuning/ethernet.cpp tuning/bluetooth.h tuning/tuning.h tuning/ethernet.h \
- tuning/tunable.cpp tuning/nl80211.h tuning/iw.c tuning/wifi.cpp tuning/tuningsysfs.h
\
- tuning/tuningsysfs.cpp tuning/wifi.h tuning/runtime.cpp tuning/tunable.h \
- tuning/runtime.h tuning/tuningusb.h tuning/iw.h calibrate/calibrate.cpp \
- calibrate/calibrate.h measurement/measurement.cpp \
- measurement/measurement.h measurement/acpi.cpp measurement/sysfs.h
measurement/sysfs.cpp \
- measurement/acpi.h measurement/extech.cpp measurement/extech.h \
- report/report-maker.cpp report/report-maker.h report/report-formatter.h \
- report/report-formatter-base.cpp report/report-formatter-base.h \
- report/report-formatter-csv.cpp report/report-formatter-csv.h \
- report/report-formatter-html.cpp report/report-formatter-html.h \
- report/report-data-html.cpp report/report-data-html.h \
- main.cpp css.h powertop.css cpu/intel_gpu.cpp \
- cpu/rapl/rapl_interface.cpp cpu/cpu_rapl_device.cpp cpu/rapl/rapl_interface.h\
- cpu/dram_rapl_device.cpp devices/gpu_rapl_device.cpp cpu/cpu_rapl_device.h \
- cpu/dram_rapl_device.h devices/gpu_rapl_device.h
-
-
-powertop_CXXFLAGS = -fno-omit-frame-pointer -fstack-protector -Wall -Wshadow -Wformat
$(NCURSES_CFLAGS) $(PCIUTILS_CFLAGS) $(LIBNL_CFLAGS) $(GLIB2_CFLAGS)
-
-
-powertop_CPPFLAGS = -D_FORTIFY_SOURCE=2 $(NCURSES_CFLAGS) $(PCIUTILS_CFLAGS)
$(LIBNL_CFLAGS) $(GLIB2_CFLAGS) $(LIBZ_CFLAGS) -DLOCALEDIR=\"$(localedir)\"
-
-powertop_LDADD = ../traceevent/libtraceevnet.la
-
-AM_LDFLAGS = $(LIBS) $(NCURSES_LIBS) $(PCIUTILS_LIBS) $(LIBNL_LIBS) $(LIBZ_LIBS)
$(PTHREAD_LIBS) $(RESOLV_LIBS)
+powertop_SOURCES = \
+ css.h \
+ devlist.cpp \
+ devlist.h \
+ display.cpp \
+ display.h \
+ lib.cpp \
+ lib.h \
+ main.cpp \
+ powertop.css \
+ \
Why the white line here?
+ calibrate/calibrate.cpp \
+ calibrate/calibrate.h \
+ cpu/abstract_cpu.cpp \
+ cpu/cpu.cpp \
+ cpu/cpu.h \
+ cpu/cpu_core.cpp \
+ cpu/cpu_linux.cpp \
+ cpu/cpu_package.cpp \
+ cpu/cpu_rapl_device.cpp \
+ cpu/cpu_rapl_device.h \
+ cpu/cpudevice.cpp \
+ cpu/cpudevice.h \
+ cpu/dram_rapl_device.cpp \
+ cpu/dram_rapl_device.h \
+ cpu/intel_cpus.cpp \
+ cpu/intel_cpus.h \
+ cpu/intel_gpu.cpp \
+ cpu/rapl/rapl_interface.cpp \
+ cpu/rapl/rapl_interface.h \
+ devices/ahci.cpp \
+ devices/ahci.h \
+ devices/alsa.cpp \
+ devices/alsa.h \
+ devices/backlight.cpp \
+ devices/backlight.h \
+ devices/device.cpp \
+ devices/device.h \
+ devices/gpu_rapl_device.cpp \
+ devices/gpu_rapl_device.h \
+ devices/i915-gpu.cpp \
+ devices/i915-gpu.h \
+ devices/network.cpp \
+ devices/network.h \
+ devices/rfkill.cpp \
+ devices/rfkill.h \
+ devices/runtime_pm.cpp \
+ devices/runtime_pm.h \
+ devices/thinkpad-fan.cpp \
+ devices/thinkpad-fan.h \
+ devices/thinkpad-light.cpp \
+ devices/thinkpad-light.h \
+ devices/usb.cpp \
+ devices/usb.h \
+ measurement/acpi.cpp \
+ measurement/acpi.h \
+ measurement/extech.cpp \
+ measurement/extech.h \
+ measurement/measurement.cpp \
+ measurement/measurement.h \
+ measurement/sysfs.cpp \
+ measurement/sysfs.h \
+ parameters/learn.cpp \
+ parameters/parameters.cpp \
+ parameters/parameters.h \
+ parameters/persistent.cpp \
+ perf/perf.cpp \
+ perf/perf.h \
+ perf/perf_bundle.cpp \
+ perf/perf_bundle.h \
+ perf/perf_event.h \
+ process/do_process.cpp \
+ process/interrupt.cpp \
+ process/interrupt.h \
+ process/powerconsumer.cpp \
+ process/powerconsumer.h \
+ process/process.cpp \
+ process/process.h \
+ process/processdevice.cpp \
+ process/processdevice.h \
+ process/timer.cpp \
+ process/timer.h \
+ process/work.cpp \
+ process/work.h \
+ report/report-data-html.cpp \
+ report/report-data-html.h \
+ report/report-formatter-base.cpp \
+ report/report-formatter-base.h \
+ report/report-formatter-csv.cpp \
+ report/report-formatter-csv.h \
+ report/report-formatter-html.cpp \
+ report/report-formatter-html.h \
+ report/report-formatter.h \
+ report/report-maker.cpp \
+ report/report-maker.h \
+ report/report.cpp \
+ report/report.h \
+ tuning/bluetooth.cpp \
+ tuning/bluetooth.h \
+ tuning/ethernet.cpp \
+ tuning/ethernet.h \
+ tuning/iw.c \
+ tuning/iw.h \
+ tuning/nl80211.h \
+ tuning/runtime.cpp \
+ tuning/runtime.h \
+ tuning/tunable.cpp \
+ tuning/tunable.h \
+ tuning/tuning.cpp \
+ tuning/tuning.h \
+ tuning/tuningsysfs.cpp \
+ tuning/tuningsysfs.h \
+ tuning/tuningusb.cpp \
+ tuning/tuningusb.h \
+ tuning/wifi.cpp \
+ tuning/wifi.h
+
+powertop_CXXFLAGS = \
+ -Wall \
+ -Wformat \
+ -Wshadow \
+ -fno-omit-frame-pointer \
+ -fstack-protector \
+ $(GLIB2_CFLAGS) \
+ $(LIBNL_CFLAGS) \
+ $(NCURSES_CFLAGS) \
+ $(PCIUTILS_CFLAGS)
+
+
+powertop_CPPFLAGS = \
+ -DLOCALEDIR=\"$(localedir)\" \
+ -D_FORTIFY_SOURCE=2 \
+ $(GLIB2_CFLAGS) \
+ $(LIBNL_CFLAGS) \
+ $(LIBZ_CFLAGS) \
+ $(NCURSES_CFLAGS) \
+ $(PCIUTILS_CFLAGS)
+
+powertop_LDADD = \
+ ../traceevent/libtraceevnet.la
+
+AM_LDFLAGS = \
+ $(LIBNL_LIBS) \
+ $(LIBS) \
+ $(LIBZ_LIBS) \
+ $(NCURSES_LIBS) \
+ $(PCIUTILS_LIBS) \
+ $(PTHREAD_LIBS) \
+ $(RESOLV_LIBS)
BUILT_SOURCES = css.h
CLEANFILES = css.h
+
css.h: powertop.css
$(SHELL) ${srcdir}/csstoh.sh ${srcdir}/powertop.css css.h
7:25 p.m.
New subject: [Powertop] [PATCH v3 04/15] clean up Makefile.am files
On 15 October 2014 00:34, Joe Konno <joe.konno(a)linux.intel.com> wrote:
On 09/07/2014 02:27 PM, Sami Kerola wrote:
[snip]
> ---
> Makefile.am | 14 +++-
[snip]
> +powertop_SOURCES = \
> + css.h \
> + devlist.cpp \
> + devlist.h \
> + display.cpp \
> + display.h \
> + lib.cpp \
> + lib.h \
> + main.cpp \
> + powertop.css \
> + \
Why the white line here?
To separate src/*.(cpp|h) file from files in sub-directory targets the
way one very clearly see the difference. Sorting all the targets in
simple alphabetical order would make the none-sub-directory objects to
be here and there in middle of listing, so separating the lists seemed
like clarity enhancement to me. If the empty line really annoys send a
patch to remove it, I don't have too strong opinions either in favor
or against it.
--
Sami Kerola
http://www.iki.fi/kerolasa/
12:03 a.m.
New subject: [Powertop] [PATCH v3 04/15] clean up Makefile.am files
On (10/15/14 09:25), Sami Kerola wrote:
>> ---
>> Makefile.am | 14 +++-
[snip]
>> +powertop_SOURCES = \
>> + css.h \
>> + devlist.cpp \
>> + devlist.h \
>> + display.cpp \
>> + display.h \
>> + lib.cpp \
>> + lib.h \
>> + main.cpp \
>> + powertop.css \
>> + \
>
> Why the white line here?
To separate src/*.(cpp|h) file from files in sub-directory targets the
way one very clearly see the difference. Sorting all the targets in
simple alphabetical order would make the none-sub-directory objects to
be here and there in middle of listing, so separating the lists seemed
like clarity enhancement to me. If the empty line really annoys send a
patch to remove it, I don't have too strong opinions either in favor
or against it.
:-)
guys, this is Makefile.am
nobody goes there.
-ss
7:27 a.m.
New subject: [Powertop] [PATCH v3 05/15] fix clang++ compilation errors
The same error was fixed in multiple files to made clang++ work.
error: variable length array of non-POD element type 'string' (aka
'basic_string<char>')
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/cpu/cpu.cpp | 18 ++++++++++++------
src/devices/ahci.cpp | 3 ++-
src/devices/device.cpp | 6 ++++--
src/devlist.cpp | 3 ++-
src/process/do_process.cpp | 9 ++++++---
src/report/report.cpp | 3 ++-
src/tuning/tuning.cpp | 10 ++++++----
7 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
index dbca20c..4e040c1 100644
--- a/src/cpu/cpu.cpp
+++ b/src/cpu/cpu.cpp
@@ -479,12 +479,12 @@ void report_display_cpu_cstates(void)
/* Tables for PKG, CORE, CPU */
pkg_tbl_size.cols=2;
pkg_tbl_size.rows= ((cstates_num+1)-LEVEL_HEADER)+1;
- string pkg_data[pkg_tbl_size.cols * pkg_tbl_size.rows];
+ string *pkg_data = new string[pkg_tbl_size.cols * pkg_tbl_size.rows];
core_tbl_size.cols=2;
core_tbl_size.rows=(cstates_num *_package->children.size())
+ _package->children.size();
- string core_data[core_tbl_size.cols * core_tbl_size.rows];
+ string *core_data = new string[core_tbl_size.cols * core_tbl_size.rows];
int num_cpus=0, num_cores=0;
for (core = 0; core < _package->children.size(); core++) {
@@ -506,7 +506,7 @@ void report_display_cpu_cstates(void)
cpu_tbl_size.cols=(2 * (num_cpus / num_cores)) + 1;
cpu_tbl_size.rows = ((cstates_num+1-LEVEL_HEADER) * _package->children.size())
+ _package->children.size();
- string cpu_data[cpu_tbl_size.cols * cpu_tbl_size.rows];
+ string *cpu_data = new string[cpu_tbl_size.cols * cpu_tbl_size.rows];
for (core = 0; core < _package->children.size(); core++) {
cpu_data[idx3]=" ";
@@ -633,6 +633,9 @@ void report_display_cpu_cstates(void)
init_cpu_table_attr(&std_table_css, title+1, cpu_tbl_size.rows,
cpu_tbl_size.cols);
report.add_table(cpu_data, &std_table_css);
+ delete [] pkg_data;
+ delete [] core_data;
+ delete [] cpu_data;
}
report.end_div();
}
@@ -688,11 +691,11 @@ void report_display_cpu_pstates(void)
/* Tables for PKG, CORE, CPU */
pkg_tbl_size.cols=2;
pkg_tbl_size.rows=((pstates_num+1)-LEVEL_HEADER)+2;
- string pkg_data[pkg_tbl_size.cols * pkg_tbl_size.rows];
+ string *pkg_data = new string[pkg_tbl_size.cols * pkg_tbl_size.rows];
core_tbl_size.cols=2;
core_tbl_size.rows=((pstates_num+2) *_package->children.size());
- string core_data[core_tbl_size.cols * core_tbl_size.rows];
+ string *core_data = new string[core_tbl_size.cols * core_tbl_size.rows];
/* PKG */
num_cpus=0;
@@ -717,7 +720,7 @@ void report_display_cpu_pstates(void)
cpu_tbl_size.cols= (num_cpus/ num_cores) + 1;
cpu_tbl_size.rows= (pstates_num+2) * _package->children.size()
+ _package->children.size();
- string cpu_data[cpu_tbl_size.cols * cpu_tbl_size.rows];
+ string *cpu_data = new string[cpu_tbl_size.cols * cpu_tbl_size.rows];
/* Core */
for (core = 0; core < _package->children.size(); core++) {
@@ -818,6 +821,9 @@ void report_display_cpu_pstates(void)
init_cpu_table_attr(&std_table_css, title,
cpu_tbl_size.rows, cpu_tbl_size.cols);
report.add_table(cpu_data, &std_table_css);
+ delete [] pkg_data;
+ delete [] core_data;
+ delete [] cpu_data;
}
report.end_div();
}
diff --git a/src/devices/ahci.cpp b/src/devices/ahci.cpp
index a4d1345..72e889f 100644
--- a/src/devices/ahci.cpp
+++ b/src/devices/ahci.cpp
@@ -384,7 +384,7 @@ void ahci_create_device_stats_table(void)
/* Set array of data in row Major order */
- string ahci_data[cols * rows];
+ string *ahci_data = new string[cols * rows];
ahci_data[0]=__("Link");
ahci_data[1]=__("Active");
ahci_data[2]=__("Partial");
@@ -398,6 +398,7 @@ void ahci_create_device_stats_table(void)
report.add_title(&title_attr, __("AHCI ALPM Residency Statistics"));
report.add_table(ahci_data, &std_table_css);
report.end_div();
+ delete [] ahci_data;
}
void ahci::report_device_stats(string *ahci_data, int idx)
diff --git a/src/devices/device.cpp b/src/devices/device.cpp
index da595a8..b4765ef 100644
--- a/src/devices/device.cpp
+++ b/src/devices/device.cpp
@@ -247,7 +247,7 @@ void show_report_devices(void)
/* Device Summary */
int summary_size=2;
- string summary[summary_size];
+ string *summary = new string[summary_size];
pw = global_joules_consumed();
char buf[32];
if (pw > 0.0001) {
@@ -263,9 +263,10 @@ void show_report_devices(void)
summary[1].append(" W");
report.add_summary_list(summary, summary_size);
}
+ delete [] summary;
/* Set array of data in row Major order */
- string device_data[cols * rows];
+ string *device_data = new string[cols * rows];
device_data[0]= __("Usage");
device_data[1]= __("Device Name");
if (show_power)
@@ -308,6 +309,7 @@ void show_report_devices(void)
/* Report Output */
report.add_title(&title_attr, __("Device Power Report"));
report.add_table(device_data, &std_table_css);
+ delete [] device_data;
}
diff --git a/src/devlist.cpp b/src/devlist.cpp
index 21b4c1c..20fe5da 100644
--- a/src/devlist.cpp
+++ b/src/devlist.cpp
@@ -326,7 +326,7 @@ void report_show_open_devices(void)
init_title_attr(&title_attr);
/* Set array of data in row Major order */
- string process_data[cols * rows];
+ string *process_data = new string[cols * rows];
sort(target->begin(), target->end(), devlist_sort);
process_data[0]=__("Process");
@@ -348,5 +348,6 @@ void report_show_open_devices(void)
/* No div attribute here inherits from device power report */
report.add_title(&title_attr, __("Process Device Activity"));
report.add_table(process_data, &std_table_css);
+ delete [] process_data;
report.end_div();
}
diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
index f7f9765..8d55507 100644
--- a/src/process/do_process.cpp
+++ b/src/process/do_process.cpp
@@ -918,7 +918,7 @@ void report_process_update_display(void)
init_title_attr(&title_attr);
/* Set array of data in row Major order */
- string software_data[cols * rows];
+ string *software_data = new string[cols * rows];
software_data[0]=__("Usage");
software_data[1]=__("Wakeups/s");
software_data[2]=__("GPU ops/s");
@@ -1013,6 +1013,7 @@ void report_process_update_display(void)
report.add_title(&title_attr, __("Overview of Software Power
Consumers"));
report.add_table(software_data, &std_table_css);
report.end_div();
+ delete [] software_data;
}
void report_summary(void)
@@ -1048,7 +1049,7 @@ void report_summary(void)
/* Set array for summary */
int summary_size =12;
- string summary[summary_size];
+ string *summary = new string [summary_size];
summary[0]=__("Target:");
summary[1]=__("1 units/s");
summary[2]=__("System: ");
@@ -1068,7 +1069,7 @@ void report_summary(void)
summary[11].append(__(" ops/s"));
/* Set array of data in row Major order */
- string summary_data[cols * (rows+1)];
+ string *summary_data = new string[cols * (rows + 1)];
summary_data[0]=__("Usage");
summary_data[1]=__("Events/s");
summary_data[2]=__("Category");
@@ -1134,6 +1135,8 @@ void report_summary(void)
report.add_title(&title_attr, __("Top 10 Power Consumers"));
report.add_table(summary_data, &std_table_css);
report.end_div();
+ delete [] summary;
+ delete [] summary_data;
}
diff --git a/src/report/report.cpp b/src/report/report.cpp
index d55531e..981e984 100644
--- a/src/report/report.cpp
+++ b/src/report/report.cpp
@@ -115,7 +115,7 @@ static void system_info(void)
init_title_attr(&title_attr);
/* Set array of data in row Major order */
- string system_data[sys_table.rows * sys_table.cols];
+ string *system_data = new string[sys_table.rows * sys_table.cols];
system_data[0]=__("PowerTOP Version");
system_data[1]=POWERTOP_VERSION;
@@ -159,6 +159,7 @@ static void system_info(void)
report.end_div();
report.end_header();
report.add_navigation();
+ delete [] system_data;
}
void init_report_output(char *filename_str, int iterations)
diff --git a/src/tuning/tuning.cpp b/src/tuning/tuning.cpp
index 6367a20..85b80ac 100644
--- a/src/tuning/tuning.cpp
+++ b/src/tuning/tuning.cpp
@@ -230,7 +230,7 @@ void report_show_tunables(void)
init_tune_table_attr(&tune_table_css, rows, cols);
/* Set array of data in row Major order */
- string tunable_data[cols * rows];
+ string *tunable_data = new string[cols * rows];
tunable_data[0]=__("Description");
tunable_data[1]=__("Script");
@@ -249,6 +249,7 @@ void report_show_tunables(void)
/* Report Output */
report.add_title(&title_attr,__("Software Settings in Need of Tuning"));
report.add_table(tunable_data, &tune_table_css);
+ delete [] tunable_data;
}
/* Second Table */
@@ -258,7 +259,7 @@ void report_show_tunables(void)
init_tune_table_attr(&tune_table_css, rows, cols);
/* Set array of data in row Major order */
- string untunable_data[rows];
+ string *untunable_data = new string[rows];
untunable_data[0]=__("Description");
for (i = 0; i < all_untunables.size(); i++)
@@ -267,6 +268,7 @@ void report_show_tunables(void)
/* Report Output */
report.add_title(&title_attr,__("Untunable Software Issues"));
report.add_table(untunable_data, &tune_table_css);
+ delete [] untunable_data;
/* Third Table */
/* Set Table attributes, rows, and cols */
@@ -275,7 +277,7 @@ void report_show_tunables(void)
init_std_table_attr(&tune_table_css, rows, cols);
/* Set array of data in row Major order */
- string tuned_data[rows];
+ string *tuned_data = new string[rows];
tuned_data[0]=__("Description");
idx=cols;
for (i = 0; i < all_tunables.size(); i++) {
@@ -291,7 +293,7 @@ void report_show_tunables(void)
report.add_title(&title_attr,__("Optimal Tuned Software Settings"));
report.add_table(tuned_data, &tune_table_css);
report.end_div();
-
+ delete [] tuned_data;
}
void clear_tuning()
--
2.1.0
5:49 a.m.
New subject: [Powertop] [PATCH v3 05/15] fix clang++ compilation errors
Hi,
On Sun, Sep 07, 2014 at 02:28:11PM -0700, powertop-request(a)lists.01.org wrote:
Date: Sun, 7 Sep 2014 22:27:08 +0100
From: Sami Kerola <kerolasa(a)iki.fi>
The same error was fixed in multiple files to made clang++ work.
error: variable length array of non-POD element type 'string' (aka
'basic_string<char>')
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
index dbca20c..4e040c1 100644
--- a/src/cpu/cpu.cpp
+++ b/src/cpu/cpu.cpp
@@ -479,12 +479,12 @@ void report_display_cpu_cstates(void)
/* Tables for PKG, CORE, CPU */
pkg_tbl_size.cols=2;
pkg_tbl_size.rows= ((cstates_num+1)-LEVEL_HEADER)+1;
- string pkg_data[pkg_tbl_size.cols * pkg_tbl_size.rows];
+ string *pkg_data = new string[pkg_tbl_size.cols * pkg_tbl_size.rows];
core_tbl_size.cols=2;
core_tbl_size.rows=(cstates_num *_package->children.size())
+ _package->children.size();
- string core_data[core_tbl_size.cols * core_tbl_size.rows];
+ string *core_data = new string[core_tbl_size.cols * core_tbl_size.rows];
int num_cpus=0, num_cores=0;
for (core = 0; core < _package->children.size(); core++) {
@@ -506,7 +506,7 @@ void report_display_cpu_cstates(void)
cpu_tbl_size.cols=(2 * (num_cpus / num_cores)) + 1;
cpu_tbl_size.rows = ((cstates_num+1-LEVEL_HEADER) * _package->children.size())
+ _package->children.size();
- string cpu_data[cpu_tbl_size.cols * cpu_tbl_size.rows];
+ string *cpu_data = new string[cpu_tbl_size.cols * cpu_tbl_size.rows];
for (core = 0; core < _package->children.size(); core++) {
cpu_data[idx3]=" ";
@@ -633,6 +633,9 @@ void report_display_cpu_cstates(void)
init_cpu_table_attr(&std_table_css, title+1, cpu_tbl_size.rows,
cpu_tbl_size.cols);
report.add_table(cpu_data, &std_table_css);
+ delete [] pkg_data;
+ delete [] core_data;
+ delete [] cpu_data;
}
report.end_div();
}
As a C++ developer with certain experience of sometimes (very) painful quality(the last
VERY PAINFUL one less than 6 weeks ago...),
I really wouldn't want to see open-coded, error-prone new/delete (or, heaven forbid,
malloc/free used in C++ code) in new (pun not intended) code anywhere -
nor non-scoped (omission-prone, return-hampered, exception-unsafe) handling
of any other, possibly more critical resources (think locks, files, ...).
Therefore I'd strongly recommend changing this patch
into using properly scoped resource handling such as std::vector
(std::array should also work, but since that is C++11 only
I'd prefer std::vector).
And possibly plus handling of std::bad_alloc cases, in this or outer scope...
Put differently, one could even say
that given an automated commit review system
any appearance of new/delete should automatically make the review fail,
with subsequent manual white-listing needed for the few rare cases
that do really want to have it (think class-internal handling of resources,
or some such).
That said, thank you very much for enabling clang++ compatibility,
and for all the other fixes!
I had wanted to venture into such areas myself, too
(e.g. for C++ diagnosis reasons), but simply haven't found the time yet.
Andreas Mohr
12:42 p.m.
New subject: [Powertop] [PATCH v3 05/15] fix clang++ compilation errors
Hi,
On Sun, Sep 07, 2014 at 02:28:11PM -0700, powertop-request(a)lists.01.org
wrote:
> Date: Sun, 7 Sep 2014 22:27:08 +0100
> From: Sami Kerola <kerolasa(a)iki.fi>
>
> The same error was fixed in multiple files to made clang++ work.
>
> error: variable length array of non-POD element type 'string' (aka
> 'basic_string<char>')
>
> Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
> index dbca20c..4e040c1 100644
> --- a/src/cpu/cpu.cpp
> +++ b/src/cpu/cpu.cpp
> @@ -479,12 +479,12 @@ void report_display_cpu_cstates(void)
> /* Tables for PKG, CORE, CPU */
> pkg_tbl_size.cols=2;
> pkg_tbl_size.rows= ((cstates_num+1)-LEVEL_HEADER)+1;
> - string pkg_data[pkg_tbl_size.cols * pkg_tbl_size.rows];
> + string *pkg_data = new string[pkg_tbl_size.cols * pkg_tbl_size.rows];
>
> core_tbl_size.cols=2;
> core_tbl_size.rows=(cstates_num *_package->children.size())
> + _package->children.size();
> - string core_data[core_tbl_size.cols * core_tbl_size.rows];
> + string *core_data = new string[core_tbl_size.cols *
> core_tbl_size.rows];
> int num_cpus=0, num_cores=0;
>
> for (core = 0; core < _package->children.size(); core++) {
> @@ -506,7 +506,7 @@ void report_display_cpu_cstates(void)
> cpu_tbl_size.cols=(2 * (num_cpus / num_cores)) + 1;
> cpu_tbl_size.rows = ((cstates_num+1-LEVEL_HEADER) *
> _package->children.size())
> + _package->children.size();
> - string cpu_data[cpu_tbl_size.cols * cpu_tbl_size.rows];
> + string *cpu_data = new string[cpu_tbl_size.cols * cpu_tbl_size.rows];
>
> for (core = 0; core < _package->children.size(); core++) {
> cpu_data[idx3]=" ";
> @@ -633,6 +633,9 @@ void report_display_cpu_cstates(void)
> init_cpu_table_attr(&std_table_css, title+1, cpu_tbl_size.rows,
> cpu_tbl_size.cols);
> report.add_table(cpu_data, &std_table_css);
> + delete [] pkg_data;
> + delete [] core_data;
> + delete [] cpu_data;
> }
> report.end_div();
> }
As a C++ developer with certain experience of sometimes (very) painful
quality(the last VERY PAINFUL one less than 6 weeks ago...),
I really wouldn't want to see open-coded, error-prone new/delete (or,
heaven forbid, malloc/free used in C++ code) in new (pun not intended)
code anywhere -
nor non-scoped (omission-prone, return-hampered, exception-unsafe)
handling
of any other, possibly more critical resources (think locks, files, ...).
Therefore I'd strongly recommend changing this patch
into using properly scoped resource handling such as std::vector
(std::array should also work, but since that is C++11 only
I'd prefer std::vector).
And possibly plus handling of std::bad_alloc cases, in this or outer
scope...
Put differently, one could even say
that given an automated commit review system
any appearance of new/delete should automatically make the review fail,
with subsequent manual white-listing needed for the few rare cases
that do really want to have it (think class-internal handling of
resources,
or some such).
That said, thank you very much for enabling clang++ compatibility,
and for all the other fixes!
I had wanted to venture into such areas myself, too
(e.g. for C++ diagnosis reasons), but simply haven't found the time yet.
Andreas Mohr
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
Andreas, feel free to submit a patch with this fix.
Thank you,
Alexandra.
9:35 p.m.
New subject: [Powertop] [PATCH v3 05/15] fix clang++ compilation errors
Hi,
On Mon, Sep 29, 2014 at 07:42:28PM -0700, Alexandra Yates wrote:
> any appearance of new/delete should automatically make the
review fail,
> with subsequent manual white-listing needed for the few rare cases
> that do really want to have it (think class-internal handling of
> resources,
> or some such).
>
>
> That said, thank you very much for enabling clang++ compatibility,
> and for all the other fixes!
> I had wanted to venture into such areas myself, too
> (e.g. for C++ diagnosis reasons), but simply haven't found the time yet.
>
> Andreas Mohr
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>
Andreas, feel free to submit a patch with this fix.
Right, that seems like a good idea
(although it's quite a bit of effort,
and all these areas have been committed now unfortunately).
I started to dive into it (did git pull of my existing
https://github.com/fenrus75/powertop ), but immediately hit a snag:
README said that I should apply the usual UNIX prayer of
./configure
make
make install
which obviously wasn't possible since in this repo-only content
the automake step was not done (likely rightfully, I think,
since these things should nicely fit the local environment,
but then it should still have been explained in the README).
Running automake then failed (again!! Somehow I knew it, since these things
magically have a tendency to fail almost always...):
# ./autogen.sh
autoreconf: Entering directory `.'
autoreconf: running: autopoint
autoreconf: running: aclocal --output=aclocal.m4t
aclocal: configure.ac: 20: macro `AM_SILENT_RULES' not found in library
aclocal: macro `AM_PROG_MKDIR_P' required but not defined
aclocal: macro `AM_PROG_MKDIR_P' required but not defined
aclocal: macro `_LT_DECL_SED' required but not defined
aclocal: macro `_LT_FUNC_STRIPNAME_CNF' required but not defined
autoreconf: aclocal failed with exit status: 1
to which https://forum.videolan.org/viewtopic.php?f=13&t=84261
had the following wise words to say: "Use automake 1.11".
Fortunately there's an automake1.11 package on Debian, however there's also of
course
the "standard", "canonical version" "automake" package (oh
darnit - which I currently
don't even have installed, oops... can you tell that I'm a CMake guy already?
;)).
So, since I only had older specific-version automake packages installed
(a sufficiently medieval automake1.7),,
I installed the generic, version-progressing one.
But it still used the old version prior to an
update-alternatives --config automake
, at which point it worked.
So, now that this is documented for other people to be helped,
on to documenting README omissions and working on the scoped array handling commit.
Andreas Mohr
12:15 a.m.
New subject: [Powertop] [PATCH v3 05/15] fix clang++ compilation errors
On Sat, 4 Oct 2014, Andreas Mohr wrote:
On Mon, Sep 29, 2014 at 07:42:28PM -0700, Alexandra Yates wrote:
>> any appearance of new/delete should automatically make the review fail,
>> with subsequent manual white-listing needed for the few rare cases
>> that do really want to have it (think class-internal handling of
>> resources,
>> or some such).
>>
>>
>> That said, thank you very much for enabling clang++ compatibility,
>> and for all the other fixes!
>> I had wanted to venture into such areas myself, too
>> (e.g. for C++ diagnosis reasons), but simply haven't found the time yet.
>>
>> Andreas Mohr
>> _______________________________________________
>> PowerTop mailing list
>> PowerTop(a)lists.01.org
>> https://lists.01.org/mailman/listinfo/powertop
>>
>
> Andreas, feel free to submit a patch with this fix.
Right, that seems like a good idea
(although it's quite a bit of effort,
and all these areas have been committed now unfortunately).
I started to dive into it (did git pull of my existing
https://github.com/fenrus75/powertop ), but immediately hit a snag:
README said that I should apply the usual UNIX prayer of
./configure
make
make install
which obviously wasn't possible since in this repo-only content
the automake step was not done (likely rightfully, I think,
since these things should nicely fit the local environment,
but then it should still have been explained in the README).
Running automake then failed (again!! Somehow I knew it, since these things
magically have a tendency to fail almost always...):
# ./autogen.sh
autoreconf: Entering directory `.'
autoreconf: running: autopoint
autoreconf: running: aclocal --output=aclocal.m4t
aclocal: configure.ac: 20: macro `AM_SILENT_RULES' not found in library
aclocal: macro `AM_PROG_MKDIR_P' required but not defined
aclocal: macro `AM_PROG_MKDIR_P' required but not defined
aclocal: macro `_LT_DECL_SED' required but not defined
aclocal: macro `_LT_FUNC_STRIPNAME_CNF' required but not defined
autoreconf: aclocal failed with exit status: 1
to which https://forum.videolan.org/viewtopic.php?f=13&t=84261
had the following wise words to say: "Use automake 1.11".
Fortunately there's an automake1.11 package on Debian, however there's also of
course
the "standard", "canonical version" "automake" package (oh
darnit - which I currently
don't even have installed, oops... can you tell that I'm a CMake guy already?
;)).
So, since I only had older specific-version automake packages installed
(a sufficiently medieval automake1.7),,
I installed the generic, version-progressing one.
But it still used the old version prior to an
update-alternatives --config automake
, at which point it worked.
So, now that this is documented for other people to be helped,
on to documenting README omissions and working on the scoped array handling commit.
Hi Andreas,
IMHO it would be better to require new enough automake as part of
autotools configuration, than add notes to README. Something like this
could do:
--->8----
From: Sami Kerola <kerolasa(a)iki.fi>
Date: Sat, 4 Oct 2014 14:54:23 +0100
Subject: [PATCH] configure: require the least automake version 1.11.1
Apparently some Debian has rather old automake, version 1.7, which is
causing build from git to fail.
Set the requird automake version to be minimum of 1.11.1, that used to be
the version GNU coreutils requied before they introduced
AM_TESTS_ENVIRONMENT that works in 1.11.2 or later. As powertop does not
have tests, the earlier GNU coreutils requirement an initial pick for
this project.
p.s. The automake 1.11.1 has been a round since December 2009, so there
really shouldn't be need for support anything older, like 1.7.9 that was
released November 2003.
Reported-by: Andreas Mohr <andi(a)lisas.de>
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
configure.ac | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 4991d97..a1dbfb8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3,7 +3,11 @@
AC_PREREQ([2.68])
AC_INIT([powertop], [2.6.1], [powertop(a)lists.01.org], [], [https://01.org/powertop])
-AM_INIT_AUTOMAKE([-Wall foreign])
+AM_INIT_AUTOMAKE([
+ -Wall
+ 1.11.2
+ foreign
+])
AC_LANG([C++])
AC_CONFIG_FILES([
Makefile
--
2.1.2
12:19 a.m.
New subject: [Powertop] [PATCH v3 05/15] fix clang++ compilation errors
On Sat, 4 Oct 2014, Sami Kerola wrote:
-AM_INIT_AUTOMAKE([-Wall foreign])
+AM_INIT_AUTOMAKE([
+ -Wall
+ 1.11.2
+ foreign
+])
Uh oh... I meant
s/1.11.2/1.11.1/
But then again even the 1.11.2 is not crazy bleeding edge, so it would
probably be a fine pick as well.
--
Sami Kerola
http://www.iki.fi/kerolasa/
7:10 a.m.
New subject: [Powertop] [PATCH v3 05/15] fix clang++ compilation errors
Hi,
On Sat, Oct 04, 2014 at 03:15:34PM +0100, Sami Kerola wrote:
On Sat, 4 Oct 2014, Andreas Mohr wrote:
>So, now that this is documented for other people to be helped,
>on to documenting README omissions and working on the scoped array handling commit.
Hi Andreas,
IMHO it would be better to require new enough automake as part of autotools
configuration, than add notes to README. Something like this could do:
Not quite, people actually need to get to know that they need automake at all
(to achieve availability of ./configure) ;)
(was mentioned shortly further below, but that's not enough)
Apparently some Debian has rather old automake, version 1.7, which
is
causing build from git to fail.
That's just my "historic" Debian installation (18 years and counting).
(bo, woody, slink, hamm or some such?)
Oh crap, my 'puter just managed to get its driver's license ;)
Another 3 years and it will be allowed to drink beer...
Set the requird automake version to be minimum of 1.11.1, that used
to be
the version GNU coreutils requied before they introduced
AM_TESTS_ENVIRONMENT that works in 1.11.2 or later. As powertop does not
have tests, the earlier GNU coreutils requirement an initial pick for
this project.
Yup, since we *do* seem to use version-specific functionality,
we likely *should* fend off version issues,
by demanding specific version requirements
in this AM_INIT_AUTOMAKE autoconf macro,
otherwise our behaviour is "improper", "weak".
p.s. The automake 1.11.1 has been a round since December 2009, so
there
really shouldn't be need for support anything older, like 1.7.9 that was
released November 2003.
Ouch, that old?
In closing: patch seems fine to me (possibly except for the specific
version number, that is).
Thanks,
Andreas Mohr
7:27 a.m.
New subject: [Powertop] [PATCH v3 06/15] remove unnecessary assignments
Found using scan-build, Clang static analyzer.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/lib.cpp | 8 ++++----
src/main.cpp | 6 ++++--
src/process/do_process.cpp | 2 +-
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/lib.cpp b/src/lib.cpp
index a8f3746..b482296 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -472,10 +472,10 @@ int read_msr(int cpu, uint64_t offset, uint64_t *value)
int fd;
char msr_path[256];
- fd = sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
+ sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
if (access(msr_path, R_OK) != 0){
- fd = sprintf(msr_path, "/dev/msr%d", cpu);
+ sprintf(msr_path, "/dev/msr%d", cpu);
if (access(msr_path, R_OK) != 0){
fprintf(stderr,
@@ -504,10 +504,10 @@ int write_msr(int cpu, uint64_t offset, uint64_t value)
int fd;
char msr_path[256];
- fd = sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
+ sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
if (access(msr_path, R_OK) != 0){
- fd = sprintf(msr_path, "/dev/msr%d", cpu);
+ sprintf(msr_path, "/dev/msr%d", cpu);
if (access(msr_path, R_OK) != 0){
fprintf(stderr,
diff --git a/src/main.cpp b/src/main.cpp
index cf4e547..99a985f 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -315,8 +315,10 @@ static void powertop_init(void)
rlmt.rlim_cur = rlmt.rlim_max = get_nr_open();
setrlimit (RLIMIT_NOFILE, &rlmt);
- ret = system("/sbin/modprobe cpufreq_stats > /dev/null 2>&1");
- ret = system("/sbin/modprobe msr > /dev/null 2>&1");
+ if (system("/sbin/modprobe cpufreq_stats > /dev/null 2>&1"))
+ fprintf(stderr, _("modprobe cpufreq_stats failed"));
+ if (system("/sbin/modprobe msr > /dev/null 2>&1"))
+ fprintf(stderr, _("modprobe msr failed"));
statfs("/sys/kernel/debug", &st_fs);
if (st_fs.f_type != (long) DEBUGFS_MAGIC) {
diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
index 8d55507..dd7d982 100644
--- a/src/process/do_process.cpp
+++ b/src/process/do_process.cpp
@@ -579,7 +579,7 @@ void perf_process_bundle::handle_trace_point(void *trace, int cpu,
uint64_t time
consumer_child_time(cpu, t);
}
else if (strcmp(event->name, "cpu_idle") == 0) {
- ret = pevent_get_field_val(NULL, event, "state", &rec, &val, 0);
+ pevent_get_field_val(NULL, event, "state", &rec, &val, 0);
if (val == (unsigned int)-1)
consume_blame(cpu);
else
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 07/15] remove unnecessary code reported by cppcheck
Return values in c_rapl_interface::rapl_measure_energy() are set, but
never read. The create_all_alsa_callback() variable is not in use. In
cursor_up() if clause had same expression in both true and false cases.
In add_runtime_tunables() filename variable was immediately overwrote.
The report_show_tunables variable srt_tmp is not in use.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/cpu/rapl/rapl_interface.cpp | 4 ++--
src/devices/alsa.cpp | 1 -
src/display.cpp | 9 ++-------
src/tuning/runtime.cpp | 4 ----
src/tuning/tuning.cpp | 1 -
5 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/src/cpu/rapl/rapl_interface.cpp b/src/cpu/rapl/rapl_interface.cpp
index a19e0a9..ab8e8ce 100644
--- a/src/cpu/rapl/rapl_interface.cpp
+++ b/src/cpu/rapl/rapl_interface.cpp
@@ -647,9 +647,9 @@ void c_rapl_interface::rapl_measure_energy()
double pp0_joules = 0;
double pp1_joules = 0;
- ret = get_pkg_power_info(&thermal_spec_power, &max_power, &min_power,
&max_time_window);
+ get_pkg_power_info(&thermal_spec_power, &max_power, &min_power,
&max_time_window);
RAPL_DBG_PRINT("Pkg Power Info: Thermal spec %f watts, max %f watts, min %f watts,
max time window %f seconds\n", thermal_spec_power, max_power, min_power,
max_time_window);
- ret = get_dram_power_info(&thermal_spec_power, &max_power, &min_power,
&max_time_window);
+ get_dram_power_info(&thermal_spec_power, &max_power, &min_power,
&max_time_window);
RAPL_DBG_PRINT("DRAM Power Info: Thermal spec %f watts, max %f watts, min %f watts,
max time window %f seconds\n", thermal_spec_power, max_power, min_power,
max_time_window);
for (;;) {
diff --git a/src/devices/alsa.cpp b/src/devices/alsa.cpp
index 5dee334..a1fca71 100644
--- a/src/devices/alsa.cpp
+++ b/src/devices/alsa.cpp
@@ -154,7 +154,6 @@ static void create_all_alsa_callback(const char *d_name)
{
char filename[4096];
class alsa *bl;
- ofstream file;
if (strncmp(d_name, "hwC", 3) != 0)
return;
diff --git a/src/display.cpp b/src/display.cpp
index c76ba27..123620a 100644
--- a/src/display.cpp
+++ b/src/display.cpp
@@ -267,13 +267,8 @@ void cursor_up(void)
if (w) {
w->cursor_up();
if(w->ypad_pos > 0) {
- if (tab_names[current_tab] == "Tunables") {
- prefresh(w->win, --w->ypad_pos, w->xpad_pos,
- 1, 0, LINES - 3, COLS - 1);
- } else {
- prefresh(w->win, --w->ypad_pos, w->xpad_pos,
- 1, 0, LINES - 3, COLS - 1);
- }
+ prefresh(w->win, --w->ypad_pos, w->xpad_pos,
+ 1, 0, LINES - 3, COLS - 1);
}
}
diff --git a/src/tuning/runtime.cpp b/src/tuning/runtime.cpp
index b57d53e..3201fdd 100644
--- a/src/tuning/runtime.cpp
+++ b/src/tuning/runtime.cpp
@@ -139,10 +139,6 @@ void add_runtime_tunables(const char *bus)
if (entry->d_name[0] == '.')
continue;
- sprintf(filename, "/sys/bus/%s/devices/%s", bus, entry->d_name);
-
-
-
sprintf(filename, "/sys/bus/%s/devices/%s/power/control", bus,
entry->d_name);
if (access(filename, R_OK) != 0)
diff --git a/src/tuning/tuning.cpp b/src/tuning/tuning.cpp
index 85b80ac..7d64cad 100644
--- a/src/tuning/tuning.cpp
+++ b/src/tuning/tuning.cpp
@@ -198,7 +198,6 @@ void report_show_tunables(void)
unsigned int i;
/* three tables; bad, unfixable, good */
sort_tunables();
- string srt_tmp;
int idx, rows = 0, cols;
/* First Table */
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 08/15] add short options to usage, and reorder options in code
Earlier user experience was pretty unintuitive. For example -h is often
help, and it generated following error message.
powertop: option requires an argument -- 'h'
This change re-assigned short option -h from --html to --help, and made
-r to be assigned with --html. All other short vs long option pairs
remain untouched.
The option data in struct option, print_usage(), getopt_long() optstring,
and getopt_long switch() case, are reorderd be in same order in all four
places. All options are in long-option alphabetical order, with
exception of --version and --help that are the last in the lists.
Some options where meant to be used as long-only options. These are made
unreachable by making the option character to be out side of range what
user can type.
Finally if an unknown option is found exit the value is changed from zero
to one to indicate an error.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/main.cpp | 162 ++++++++++++++++++++++++++++++-----------------------------
1 file changed, 82 insertions(+), 80 deletions(-)
diff --git a/src/main.cpp b/src/main.cpp
index 99a985f..e4d5d31 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -69,22 +69,28 @@ int debug_learning = 0;
unsigned time_out = 20;
int leave_powertop = 0;
+enum {
+ OPT_AUTO_TUNE = CHAR_MAX + 1,
+ OPT_EXTECH,
+ OPT_DEBUG
+};
+
static const struct option long_options[] =
{
/* These options set a flag. */
- {"debug", no_argument, &debug_learning, 'd'},
- {"version", no_argument, NULL, 'V'},
- {"help",no_argument, NULL, 'u'}, /* u for usage */
- {"calibrate",no_argument, NULL, 'c'},
- {"auto-tune",no_argument, NULL, 'a'},
- {"html", optional_argument, NULL, 'h'},
- {"csv", optional_argument, NULL, 'C'},
- {"extech", optional_argument, NULL, 'e'},
- {"time", optional_argument, NULL, 't'},
- {"iteration", optional_argument, NULL, 'i'},
- {"workload", optional_argument, NULL, 'w'},
- {"quiet", no_argument, NULL, 'q'},
- {NULL, 0, NULL, 0}
+ {"auto-tune", no_argument, NULL, OPT_AUTO_TUNE},
+ {"calibrate", no_argument, NULL, 'c'},
+ {"csv", optional_argument, NULL, 'C'},
+ {"debug", no_argument, &debug_learning, OPT_DEBUG},
+ {"extech", optional_argument, NULL, OPT_EXTECH},
+ {"html", optional_argument, NULL, 'r'},
+ {"iteration", optional_argument, NULL, 'i'},
+ {"quiet", no_argument, NULL, 'q'},
+ {"time", optional_argument, NULL, 't'},
+ {"workload", optional_argument, NULL, 'w'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, 'h'},
+ {NULL, 0, NULL, 0}
};
static void print_version()
@@ -108,21 +114,21 @@ static bool set_refresh_timeout()
static void print_usage()
{
- printf("%s\n\n",_("Usage: powertop [OPTIONS]"));
- printf("--debug \t\t %s\n",_("run in \"debug\" mode"));
- printf("--version \t\t %s\n",_("print version information"));
- printf("--calibrate \t\t %s\n",_("runs powertop in calibration
mode"));
- printf("--auto-tune \t\t %s\n",_("Sets all tunable options to their GOOD
setting"));
- printf("--extech%s \t %s\n",_("[=devnode]"),_("uses an Extech
Power Analyzer for measurements"));
- printf("--html%s \t %s\n",_("[=FILENAME]"),_("generate a html
report"));
- printf("--csv%s \t %s\n",_("[=FILENAME]"),_("generate a csv
report"));
- printf("--time%s \t %s\n",_("[=seconds]"), _("generate a report
for 'x' seconds"));
- printf("--iteration%s\n", _("[=iterations] number of times to run each
test"));
- printf("--workload%s \t %s\n", _("[=workload]"), _("file to
execute for workload"));
- printf("--quiet \t\t %s\n", _("suppress stderr output"));
- printf("--help \t\t\t %s\n",_("print this help menu"));
+ printf("%s\n\n", _("Usage: powertop [OPTIONS]"));
+ printf(" --auto-tune\t %s\n", _("sets all tunable options to their
GOOD setting"));
+ printf(" -c, --calibrate\t %s\n", _("runs powertop in calibration
mode"));
+ printf(" -C, --csv%s\t %s\n", _("[=filename]"), _("generate a
csv report"));
+ printf(" --debug\t\t %s\n", _("run in \"debug\"
mode"));
+ printf(" --extech%s\t %s\n", _("[=devnode]"), _("uses an
Extech Power Analyzer for measurements"));
+ printf(" -r, --html%s\t %s\n", _("[=filename]"), _("generate a
html report"));
+ printf(" -i, --iteration%s\n", _("[=iterations] number of times to run
each test"));
+ printf(" -q, --quiet\t\t %s\n", _("suppress stderr output"));
+ printf(" -t, --time%s\t %s\n", _("[=seconds]"), _("generate a
report for 'x' seconds"));
+ printf(" -w, --workload%s %s\n", _("[=workload]"), _("file to
execute for workload"));
+ printf(" -V, --version\t\t %s\n", _("print version information"));
+ printf(" -h, --help\t\t %s\n", _("print this help menu"));
printf("\n");
- printf("%s\n\n",_("For more help please refer to the README"));
+ printf("%s\n\n", _("For more help please refer to the 'man 8
powertop'"));
}
static void do_sleep(int seconds)
@@ -380,63 +386,59 @@ int main(int argc, char **argv)
#endif
while (1) { /* parse commandline options */
- c = getopt_long (argc, argv, "ch:C:i:t:uVw:q", long_options,
&option_index);
+ c = getopt_long(argc, argv, "cC:r:i:qt:w:Vh", long_options,
&option_index);
/* Detect the end of the options. */
if (c == -1)
break;
-
switch (c) {
- case 'V':
- print_version();
- exit(0);
- break;
-
- case 'e': /* Extech power analyzer support */
- checkroot();
- extech_power_meter(optarg ? optarg : "/dev/ttyUSB0");
- break;
- case 'u':
- print_usage();
- exit(0);
- break;
- case 'a':
- auto_tune = 1;
- leave_powertop = 1;
- break;
- case 'c':
- powertop_init();
- calibrate();
- break;
-
- case 'h': /* html report */
- reporttype = REPORT_HTML;
- sprintf(filename, "%s", optarg ? optarg : "powertop.html" );
- break;
-
- case 't':
- time_out = (optarg ? atoi(optarg) : 20);
- break;
-
- case 'i':
- iterations = (optarg ? atoi(optarg) : 1);
- break;
-
- case 'w': /* measure workload */
- sprintf(workload, "%s", optarg ? optarg :'\0' );
- break;
- case 'q':
- if(freopen("/dev/null", "a", stderr))
- fprintf(stderr, _("Quite mode failed!\n"));
- break;
-
- case 'C': /* csv report*/
- reporttype = REPORT_CSV;
- sprintf(filename, "%s", optarg ? optarg : "powertop.csv");
- break;
- case '?': /* Unknown option */
- /* getopt_long already printed an error message. */
- exit(0);
- break;
+ case OPT_AUTO_TUNE:
+ auto_tune = 1;
+ leave_powertop = 1;
+ break;
+ case 'c':
+ powertop_init();
+ calibrate();
+ break;
+ case 'C': /* csv report */
+ reporttype = REPORT_CSV;
+ sprintf(filename, "%s", optarg ? optarg : "powertop.csv");
+ break;
+ case OPT_DEBUG:
+ /* implemented using getopt_long(3) flag */
+ break;
+ case OPT_EXTECH: /* Extech power analyzer support */
+ checkroot();
+ extech_power_meter(optarg ? optarg : "/dev/ttyUSB0");
+ break;
+ case 'r': /* html report */
+ reporttype = REPORT_HTML;
+ sprintf(filename, "%s", optarg ? optarg : "powertop.html");
+ break;
+ case 'i':
+ iterations = (optarg ? atoi(optarg) : 1);
+ break;
+ case 'q':
+ if (freopen("/dev/null", "a", stderr))
+ fprintf(stderr, _("Quite mode failed!\n"));
+ break;
+ case 't':
+ time_out = (optarg ? atoi(optarg) : 20);
+ break;
+ case 'w': /* measure workload */
+ sprintf(workload, "%s", optarg ? optarg : '\0');
+ break;
+ case 'V':
+ print_version();
+ exit(0);
+ break;
+ case 'h':
+ print_usage();
+ exit(0);
+ break;
+ case '?': /* Unknown option */
+ /* getopt_long already printed an error message. */
+ exit(1);
+ break;
}
}
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 09/15] do not use ncurses with --auto-tune
The --auto-tune is not expected to be interactive, so all fiddling with
ncurses can and should be skipped. This also makes the --auto-tune more
suitable to be used in script(s).
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/main.cpp | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/main.cpp b/src/main.cpp
index e4d5d31..0ae0b87 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -460,7 +460,8 @@ int main(int argc, char **argv)
end_pci_access();
exit(0);
}
- init_display();
+ if (!auto_tune)
+ init_display();
initialize_tuning();
/* first one is short to not let the user wait too long */
one_measurement(1, NULL);
@@ -473,11 +474,13 @@ int main(int argc, char **argv)
}
while (!leave_powertop) {
- show_cur_tab();
+ if (!auto_tune)
+ show_cur_tab();
one_measurement(time_out, NULL);
learn_parameters(15, 0);
}
- endwin();
+ if (!auto_tune)
+ endwin();
printf("%s\n", _("Leaving PowerTOP"));
end_process_data();
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 10/15] improve manual groff syntax
Use header macro .TH expected way. Use English spacing, that means two
spaces after a dot. Use simple font macros, such as .B or .BR, where
possible rather than \fB inline selectors. Highlight option arguments
consistently, also in description segments. Convert interactive commands
to a table. Use mail and web address macros when appropriate.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
doc/powertop.8 | 141 +++++++++++++++++++++++++++++----------------------------
1 file changed, 71 insertions(+), 70 deletions(-)
diff --git a/doc/powertop.8 b/doc/powertop.8
index 3535a21..31e7e6d 100644
--- a/doc/powertop.8
+++ b/doc/powertop.8
@@ -1,94 +1,95 @@
-.TH POWERTOP 8 "June 1, 2012" "Linux" "powertop manual"
+.TH POWERTOP "8" "2014-08-02" "powertop manual"
"System Administration"
.SH NAME
powertop \- a power consumption and power management diagnosis tool.
.SH SYNOPSIS
.B powertop
-.RB [
-.IR options
-.RB ]
+.RI [ options ]
.SH DESCRIPTION
-.P
-\fBpowertop\fP is a program that helps to diagnose various issues with power consumption
-and power management. It also has an interactive mode allowing one to
-experiment with various power management settings. When invoking powertop
-without arguments powertop starts in interactive mode.
+.B powertop
+is a program that helps to diagnose various issues with power consumption
+and power management. It also has an interactive mode allowing one to
+experiment with various power management settings. When invoking
+powertop without arguments powertop starts in interactive mode.
.SH OPTIONS
.TP
-\fB\-\-calibrate
-runs powertop in calibration mode. When running on battery, powertop
-can track power consumption as well as system activity. When there
-are enough measurements, powertop can start to report power estimates. One
+.B \-\-calibrate
+Runs powertop in calibration mode. When running on battery, powertop can
+track power consumption as well as system activity. When there are
+enough measurements, powertop can start to report power estimates. One
can get more accurate estimates by using this option to enable a
-calibration cycle. This will cycle through various display levels and USB
-device activities and workloads.
+calibration cycle. This will cycle through various display levels and
+USB device activities and workloads.
.TP
-\fB\-\-csv[\fR=\fIFILENAME\fR]
-generate a CSV report. If a filename is not specified then the
-default name "powertop.csv" is used. The CSV report can be
-used for reporting and data analysis.
+\fB\-\-csv\fR[=\fIfilename\fR]
+Generate a CSV report. If a
+.I filename
+is not specified then the default name
+.B powertop.csv
+is used. The CSV report can be used for reporting and data analysis.
.TP
-\fB\-\-debug
- run in "debug" mode.
+.B \-\-debug
+Run in "debug" mode.
.TP
\fB\-\-extech\fR=\fIdevnode\fR
-use the Extech Power Analyzer for measurements. This allows one to
+Use the Extech Power Analyzer for measurements. This allows one to
specify the serial device node of the serial to USB adaptor connecting to
-the Extech Power Analyzer, for example /dev/ttyUSB0.
+the Extech Power Analyzer, for example
+.IR /dev/ttyUSB0 .
.TP
-\fB\-\-help
- show the help message.
+.B \-\-help
+Show the help message.
.TP
-\fB\-\-html[\fR=\fIFILENAME\fR]
-generate an HTML report. If a filename is not specified then the
-default name "powertop.html" is used. The HTML report can be
-sent to others to help diagnose power issues.
+\fB\-\-html\fR[=\fIfilename\fR]
+Generate an HTML report. If a
+.I filename
+is not specified then the default name
+.B powertop.html
+is used. The HTML report can be sent to others to help diagnose power
+issues.
.TP
-\fB\-\-iteration[\fR=\fIiterations\fR]
-number of times to run each test.
+\fB\-\-iteration\fR[=\fIiterations\fR]
+Number of times to run each test.
.TP
-\fB\-\-quiet
- suppress stderr output.
+.B \-\-quiet
+Suppress stderr output.
.TP
-\fB\-\-time[\fR=\fIseconds\fR]
-generate a report for a specified number of seconds.
+\fB\-\-time\fR[=\fIseconds\fR]
+Generate a report for a specified number of
+.IR seconds .
.TP
-\fB\-\-version
-print version information and exit.
+.B \-\-version
+Print version information and exit.
.SH COMMANDS
In interactive mode, the following key bindings are available:
-.TP
-\fBTab
-Show next tab
-.TP
-\fBBackTab
-Show previous tab
-.TP
-\fBRight Arrow
-Scroll to the right
-.TP
-\fBLeft Arrow
-Scroll to the left
-.TP
-\fBUp Arrow, PageUp
-Scroll up or select previous item
-.TP
-\fBDown Arrow, PageDown
-Scroll down or select next item
-.TP
-\fBSpace, Return
-Activate current item
-.TP
-\fBs
-Set refresh timeout
-.TP
-\fBr
-Refresh window
-.TP
-\fBq, Ctrl-C, Escape
-Exit powertop
+.IP
+.TS
+tab(@);
+l l.
+\fBTab\fR@Show next tab
+\fBBackTab\fR@Show previous tab
+\fBRight Arrow\fR@Scroll to the right
+\fBLeft Arrow\fR@Scroll to the left
+\fBUp Arrow\fR, \fBPageUp\fR@Scroll up or select previous item
+\fBDown Arrow\fR, \fBPageDown\fR@Scroll down or select next item
+\fBSpace\fR, \fBReturn\fR@Activate current item
+\fBs\fR@Set refresh timeout
+\fBr\fR@Refresh window
+\fBq\fR, \fBCtrl-C\fR, \fBEscape\fR@Exit powertop
+.TE
.SH BUGS
-Send bug reports to <powertop(a)lists.01.org>
+Send bug reports to
+.MT powertop(a)lists.01.org
+.ME
.SH SEE ALSO
-The program is more fully described at https://01.org/powertop
+The program is more fully described at
+.UR https://01.org/powertop
+.UE
.SH AUTHOR
-powertop was written by Arjan van de Ven <arjan(a)linux.intel.com>, and is currently
maintained by Chris E Ferron <chris.e.ferron(a)linux.intel.com>.
+powertop was written by
+.MT arjan@\:linux.\:intel.\:com
+Arjan van de Ven
+.ME ,
+and is currently maintained by
+.MT chris.\:e.\:ferron@\:linux.\:intel.\:com
+Chris E Ferron
+.ME .
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 11/15] fix couple typos
Found with misspell-check.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/tuning/nl80211.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/tuning/nl80211.h b/src/tuning/nl80211.h
index e4a8839..83b0514 100644
--- a/src/tuning/nl80211.h
+++ b/src/tuning/nl80211.h
@@ -828,7 +828,7 @@ enum nl80211_commands {
* bit. Depending on which antennas are selected in the bitmap, 802.11n
* drivers can derive which chainmasks to use (if all antennas belonging to
* a particular chain are disabled this chain should be disabled) and if
- * a chain has diversity antennas wether diversity should be used or not.
+ * a chain has diversity antennas whether diversity should be used or not.
* HT capabilities (STBC, TX Beamforming, Antenna selection) can be
* derived from the available chains after applying the antenna mask.
* Non-802.11n drivers can derive wether to use diversity or not.
@@ -855,7 +855,7 @@ enum nl80211_commands {
* @__NL80211_ATTR_AFTER_LAST: internal use
*/
enum nl80211_attrs {
-/* don't change the order or add anything inbetween, this is ABI! */
+/* don't change the order or add anything between, this is ABI! */
NL80211_ATTR_UNSPEC,
NL80211_ATTR_WIPHY,
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 12/15] wrap lines in README.traceevent
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
README.traceevent | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/README.traceevent b/README.traceevent
index 38563ab..0747766 100644
--- a/README.traceevent
+++ b/README.traceevent
@@ -1,15 +1,20 @@
traceevent:
-The traceevent library being built in PowerTOP is really a effort by Steven Rostedt.
-The long term intent is for Steven to push the trace event library to distributions, and
consume it externally. Right now the PowerTOP project is keeping in sync with his code
posted in the Linux kernel git under tools/lib/traceevent.
-We will not take patches into the traceevent code directly, rather we will be re-basing
-against that code base.
+The traceevent library being built in PowerTOP is really a effort by
+Steven Rostedt. The long term intent is for Steven to push the trace
+event library to distributions, and consume it externally. Right now the
+PowerTOP project is keeping in sync with his code posted in the Linux
+kernel git under tools/lib/traceevent. We will not take patches into the
+traceevent code directly, rather we will be re-basing against that code
+base.
If you find a bug in the trace event code, you should to one of two things.
-1. Send a patch to the PowerTOP mailing list, which will get forwared to Steven if
appropriate.
-2. Send a patch to the PowerTOP mailing list, cc <Steven Rostedt>
rostedt(a)goodmis.org
+1. Send a patch to the PowerTOP mailing list, which will get forwared to
+ Steven if appropriate.
+2. Send a patch to the PowerTOP mailing list, and
+ CC: Steven Rostedt <rostedt(a)goodmis.org>
-Please do use the PowerTOP mailing list to discuss topics about traceevent as its used
with PowerTOP.
-
+Please do use the PowerTOP mailing list to discuss topics about
+traceevent as its used with PowerTOP.
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 13/15] clean up .gitignore file
Keep the regular expressions, directories, and individual files separate
understandable way.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
.gitignore | 111 +++++++++++++++++++++++++++----------------------------------
1 file changed, 49 insertions(+), 62 deletions(-)
diff --git a/.gitignore b/.gitignore
index d00973f..b9188a1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,66 +1,53 @@
-*~
-*.o
+# regular expressions
*.a
-*.html
*.csv
-*.powertop
-ChangeLog
-Makefile.in
-aclocal.m4
-config.guess
-config.h.in
-config.rpath
-config.sub
-configure
-depcomp
-install-sh
-ltmain.sh
-missing
-stamp-h1
-Makefile
-config.h
-config.log
-config.status
-libtool
-pevent/.deps/
-pevent/Makefile
-pevent/Makefile.in
-po/Makefile
-po/Makevars.template
-po/POTFILES
-po/Rules-quot
-po/boldquot.sed
-po/en(a)boldquot.header
-po/en(a)quot.header
-po/insert-header.sin
-po/quot.sed
-po/remove-potcdate.sin
-po/*.gmo
-po/stamp-po
-src/powertop
-src/css.h
-src/Makefile.in
-src/.deps/
-src/Makefile
-src/calibrate/.deps/
-src/cpu/.deps/
-src/cpu/rapl/.deps
-src/devices/.deps/
-src/measurement/.deps/
-src/parameters/.deps/
-src/perf/.deps/
-src/process/.deps/
-src/tuning/.deps/
-src/report/.deps/
-m4
*.dirstamp
-*.lo
+*.html
*.la
-autom4te.cache
-remove-potcdate.sed
-compile
-stamp-po
-ABOUT-NLS
-ar-lib
-traceevent/.deps/
-traceevents/.libs/
+*.lo
+*.o
+*.powertop
+*~
+.deps
+/po/*.gmo
+Makefile
+Makefile.in
+
+# directories
+/m4/
+
+# individual files
+/ABOUT-NLS
+/ChangeLog
+/aclocal.m4
+/ar-lib
+/autom4te.cache
+/compile
+/config.guess
+/config.h
+/config.h.in
+/config.log
+/config.rpath
+/config.status
+/config.sub
+/configure
+/depcomp
+/install-sh
+/libtool
+/ltmain.sh
+/missing
+/po/Makevars.template
+/po/POTFILES
+/po/Rules-quot
+/po/boldquot.sed
+/po/en(a)boldquot.header
+/po/en(a)quot.header
+/po/insert-header.sin
+/po/quot.sed
+/po/remove-potcdate.sin
+/po/stamp-po
+/remove-potcdate.sed
+/src/css.h
+/src/powertop
+/stamp-h1
+/stamp-po
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 14/15] add --auto-tune and --workload to manual page
Options in manual page are now in same order as in usage output, and they
include short options when aplicable.
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
doc/powertop.8 | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/doc/powertop.8 b/doc/powertop.8
index 31e7e6d..60e3c73 100644
--- a/doc/powertop.8
+++ b/doc/powertop.8
@@ -12,7 +12,10 @@ experiment with various power management settings. When invoking
powertop without arguments powertop starts in interactive mode.
.SH OPTIONS
.TP
-.B \-\-calibrate
+.B \-\-auto\-tune
+Set all tunable options to their good setting without interaction.
+.TP
+.BR \-c ", " \-\-calibrate
Runs powertop in calibration mode. When running on battery, powertop can
track power consumption as well as system activity. When there are
enough measurements, powertop can start to report power estimates. One
@@ -20,7 +23,7 @@ can get more accurate estimates by using this option to enable a
calibration cycle. This will cycle through various display levels and
USB device activities and workloads.
.TP
-\fB\-\-csv\fR[=\fIfilename\fR]
+\fB\-C\fR, \fB\-\-csv\fR[=\fIfilename\fR]
Generate a CSV report. If a
.I filename
is not specified then the default name
@@ -28,7 +31,7 @@ is not specified then the default name
is used. The CSV report can be used for reporting and data analysis.
.TP
.B \-\-debug
-Run in "debug" mode.
+Run in debug mode.
.TP
\fB\-\-extech\fR=\fIdevnode\fR
Use the Extech Power Analyzer for measurements. This allows one to
@@ -36,10 +39,7 @@ specify the serial device node of the serial to USB adaptor connecting
to
the Extech Power Analyzer, for example
.IR /dev/ttyUSB0 .
.TP
-.B \-\-help
-Show the help message.
-.TP
-\fB\-\-html\fR[=\fIfilename\fR]
+\fB\-r\fR, \fB\-\-html\fR[=\fIfilename\fR]
Generate an HTML report. If a
.I filename
is not specified then the default name
@@ -47,18 +47,26 @@ is not specified then the default name
is used. The HTML report can be sent to others to help diagnose power
issues.
.TP
-\fB\-\-iteration\fR[=\fIiterations\fR]
+\fB\-i\fR, \fB\-\-iteration\fR[=\fIiterations\fR]
Number of times to run each test.
.TP
-.B \-\-quiet
+.BR \-q ", " \-\-quiet
Suppress stderr output.
.TP
-\fB\-\-time\fR[=\fIseconds\fR]
+\fB\-t\fR, \fB\-\-time\fR[=\fIseconds\fR]
Generate a report for a specified number of
.IR seconds .
.TP
-.B \-\-version
+\fB\-w\fR, \fB\-\-workload\fR[=\fIworkload\fR]
+Execute
+.I workload
+file as a part of calibration before making a report.
+.TP
+.BR \-V ", " \-\-version
Print version information and exit.
+.TP
+.BR \-h ", " \-\-help
+Show the help message and exit.
.SH COMMANDS
In interactive mode, the following key bindings are available:
.IP
--
2.1.0
7:27 a.m.
New subject: [Powertop] [PATCH v3 15/15] allow none-ncurses messaging with ui_notify_user()
The ui_notify_user() is made to be a function pointer, and depending on
need it will either send messages using ncurses functions or simply print
to console. At the moment the ui_notify_user_console() is not reachable,
but that might change in future.
Proposed-by: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/lib.cpp | 11 ++++++++++-
src/lib.h | 4 +++-
src/main.cpp | 4 +++-
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/lib.cpp b/src/lib.cpp
index b482296..cce7973 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -531,7 +531,7 @@ int write_msr(int cpu, uint64_t offset, uint64_t value)
#define UI_NOTIFY_BUFF_SZ 2048
-void ui_notify_user(const char *frmt, ...)
+void ui_notify_user_ncurses(const char *frmt, ...)
{
char notify[UI_NOTIFY_BUFF_SZ];
va_list list;
@@ -548,3 +548,12 @@ void ui_notify_user(const char *frmt, ...)
mvprintw(1, 0, notify);
attroff(COLOR_PAIR(1));
}
+
+void ui_notify_user_console(const char *frmt, ...)
+{
+ va_list list;
+
+ va_start(list, frmt);
+ vprintf(frmt, list);
+ va_end(list);
+}
diff --git a/src/lib.h b/src/lib.h
index 1d9fa8a..ec50946 100644
--- a/src/lib.h
+++ b/src/lib.h
@@ -83,5 +83,7 @@ extern int write_msr(int cpu, uint64_t offset, uint64_t value);
extern void align_string(char *buffer, size_t min_sz, size_t max_sz);
-extern void ui_notify_user(const char *frmt, ...);
+extern void ui_notify_user_ncurses(const char *frmt, ...);
+extern void ui_notify_user_console(const char *frmt, ...);
+extern void (*ui_notify_user) (const char *frmt, ...);
#endif
diff --git a/src/main.cpp b/src/main.cpp
index 0ae0b87..d3963ed 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -68,6 +68,7 @@
int debug_learning = 0;
unsigned time_out = 20;
int leave_powertop = 0;
+void (*ui_notify_user) (const char *frmt, ...);
enum {
OPT_AUTO_TUNE = CHAR_MAX + 1,
@@ -384,7 +385,7 @@ int main(int argc, char **argv)
bindtextdomain (PACKAGE, LOCALEDIR);
textdomain (PACKAGE);
#endif
-
+ ui_notify_user = ui_notify_user_ncurses;
while (1) { /* parse commandline options */
c = getopt_long(argc, argv, "cC:r:i:qt:w:Vh", long_options,
&option_index);
/* Detect the end of the options. */
@@ -394,6 +395,7 @@ int main(int argc, char **argv)
case OPT_AUTO_TUNE:
auto_tune = 1;
leave_powertop = 1;
+ ui_notify_user = ui_notify_user_console;
break;
case 'c':
powertop_init();
--
2.1.0
6:22 a.m.
New subject: [Powertop] [PATCH v3 00/15] pull: third time lucky
On 7 September 2014 22:27, Sami Kerola <kerolasa(a)iki.fi> wrote:
The following 15 patches are re-submission.
[snip]
are available in the git repository at:
git://github.com/kerolasa/powertop.git sami
for you to fetch changes up to 295c86c4f5fa22024792e8e850f303eec7f4b6a3:
allow none-ncurses messaging with ui_notify_user() (2014-09-07 21:52:48 +0100)
----------------------------------------------------------------
Sami Kerola (15):
configure: use vertical lists
configure: prefer AS_IF macro rather than shell if statement
add src/cpu/rapl/.deps to .gitignore
clean up Makefile.am files
fix clang++ compilation errors
remove unnecessary assignments
remove unnecessary code reported by cppcheck
add short options to usage, and reorder options in code
do not use ncurses with --auto-tune
improve manual groff syntax
fix couple typos
wrap lines in README.traceevent
clean up .gitignore file
add --auto-tune and --workload to manual page
allow none-ncurses messaging with ui_notify_user()
Good evening,
It's been a while since I sent the patches & pull. Getting either
feedback, merge, or reject would be nice.
--
Sami Kerola
http://www.iki.fi/kerolasa/
12:40 p.m.
New subject: [Powertop] [PATCH v3 00/15] pull: third time lucky
Hello,
The following 15 patches are re-submission. Reply indicators are related
to feedback from:
https://lists.01.org/pipermail/powertop/2014-August/001495.html
> 0- When creating your patches modify the patch set subject using: git
> format-patch --cover-letter --subject-prefix="PATCH Vx"... Where x
> is
> the next version for your patches. For your next version use v3.
> This very useful for sorting purposes on my inbox.
That makes sense. Done.
> 1- Every time you make changes to your patches you would need to send
> the entire set of patches with an updated version. This link
> explains patch best practices that we follow on PowerTOP and across
> the Linux kernel community:
> http://kernelnewbies.org/OPWfirstpatch#head-067c0e555973ae364180b2e3fbe90...
Are you sure kernel newbies page is telling to resubmit long patch sets
when there is only modest change is some in middle? IMHO resubmitting
individual patches that has changed when combined with a git remote
repository containing the full patch set is good way to do keep noise
level low.
But then again all projects has some habits that I don't understand/agree
with which shouldn't really be a problem. I will keep on resubmitting
patch sets when requested to do so (and protest a bit by having the git
remote available ;-)
> 3- It is a good practice to add the names of the people who reviewed
> your patches, at the commit paragraph Reviewed_by:[the name of the
> person].
Done.
> Here is my review to the 14 patches you sent on v2 of your code:
>
> P1, P2, P4, P5, P6, P10, P11, P12, P13, and P14:
> -Look good.
Great.
> P3:
> - Change the subject line to something more descriptive. Add a
> description of why are you making this change, and how it benefits
> PowerTOP. The subject lines and descriptions are very useful when
> bisecting the tree for debugging purposes.
Done: add src/cpu/rapl/.deps to .gitignore
> P7:
> - Please add a description to this patch. (Same reasoning as on P3)
See the patch 0007.
> P8:
> - I have a problem naming -h --html and -u --help. Please change -h to
> --help, and use another letter for --html for instance: r as of
> report would be more fitting. New users will want to do powertop -h
> and see the help not the html report and get totally lost. It is
> just matter of reusing the already established usability metaphors.
I did change -h to --help, which is ABI change to undocumented interface,
which may or may not better than having unconventional short option
assignment. BTW I think it would be good to add this change to release
notes, just that no-one can later tell breaking changes happen without
noise.
> P9:
> - Please abbreviate the subject and add a description to this patch.
Done.
> - To make this a feature-complete Sergey's feedback should be
> implemented: "...to extend ui_notify_user() so it can cover both
> cases: initialised ncurses -- calling mvprintw(), and uninitialised
> ncurses -- printf (to stdout). this will change a bit 'powertop
> --auto-tune' behaviour, since by default user will now see a 'list'
> of commands executed by powertop, but it's really easy to silent
> powertop via stdout redirection".
As Sergey mentioned the mvprintw() avoidance is not needed. But since I
figured out how to make both cases, ncurses and console, work without
pain a patch 0014 was added to the change set. At the moment one can say
usefulness of the change is close to zero, as the ui_notify_user() is not
reached in case of --auto-tune, but perhaps that changes in future. And
then it's nice to have a facility in place that just works.
The following changes since commit
cdff23d6e40e31a68286a51e3c94c8a901a74091:
powertop: remove temporary file (2014-08-22 09:18:34 -0700)
are available in the git repository at:
git://github.com/kerolasa/powertop.git sami
for you to fetch changes up to 295c86c4f5fa22024792e8e850f303eec7f4b6a3:
allow none-ncurses messaging with ui_notify_user() (2014-09-07 21:52:48
+0100)
----------------------------------------------------------------
Sami Kerola (15):
configure: use vertical lists
configure: prefer AS_IF macro rather than shell if statement
add src/cpu/rapl/.deps to .gitignore
clean up Makefile.am files
fix clang++ compilation errors
remove unnecessary assignments
remove unnecessary code reported by cppcheck
add short options to usage, and reorder options in code
do not use ncurses with --auto-tune
improve manual groff syntax
fix couple typos
wrap lines in README.traceevent
clean up .gitignore file
add --auto-tune and --workload to manual page
allow none-ncurses messaging with ui_notify_user()
.gitignore | 110 ++++++++++-------------
Makefile.am | 14 ++-
README.traceevent | 21 +++--
configure.ac | 102 ++++++++++++++++-----
doc/powertop.8 | 149 ++++++++++++++++---------------
src/Makefile.am | 193
+++++++++++++++++++++++++++++++---------
src/cpu/cpu.cpp | 18 ++--
src/cpu/rapl/rapl_interface.cpp | 4 +-
src/devices/ahci.cpp | 3 +-
src/devices/alsa.cpp | 1 -
src/devices/device.cpp | 6 +-
src/devlist.cpp | 3 +-
src/display.cpp | 9 +-
src/lib.cpp | 19 ++--
src/lib.h | 4 +-
src/main.cpp | 181
+++++++++++++++++++------------------
src/process/do_process.cpp | 11 ++-
src/report/report.cpp | 3 +-
src/tuning/nl80211.h | 4 +-
src/tuning/runtime.cpp | 4 -
src/tuning/tuning.cpp | 11 +--
21 files changed, 533 insertions(+), 337 deletions(-)
--
2.1.0
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
Hi Sami,
Thank you very much for your patches they made it today to our upstream tree.
Thank you,
Alexandra.
2895
days inactive
2933
days old
29 comments
5 participants
participants (5)
-
Alexandra Yates -
Andreas Mohr -
Joe Konno -
Sami Kerola -
Sergey Senozhatsky