Created attachment 7731 111 - makes valgrind happy: init all mem before use; kvm_getargv() -> sysctl() - change double to float All platforms: - get_task_list() do zeromem for all Task~s element before add to return array - replace const size to sizeof(), ex.: g_snprintf (task->name, 256, "foo"); -> g_snprintf (task->name, sizeof(task->name), "foo");
Looks good, the problem is the same of Bug 14403, it does too much in a single patch (and would become a single commit). If you have the availability, isolate the small improvements (G_DEBUG_FMG, sizeof, float/double stuff and etc) from your patches into one, and keep the significant improvements in their patches/bug reports. That way it's easier for the current maintainer (Simon?) to push your changes.
i dont really understand why you switch from kvm_getargv to sysctl w/ KERN_PROC_ARGS .. what does this actually improve ? Why switching from double to float ? precision ?
Oh and btw, memset(foo, 0x00, size) is shorter when written as bzero(foo, size) :)
rim referenced this bugreport in commit 5e0fb6f4bcf2935fd1bd38336947d36f642d565d Use sizeof() when appropriate instead of hardcoding values (bug 14401, bug 14403) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=5e0fb6f4bcf2935fd1bd38336947d36f642d565d
rim referenced this bugreport in commit c725dee6a9a4090151cb4251c935ba002fad071b more sizeof (bug 14401, bug 14403) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=c725dee6a9a4090151cb4251c935ba002fad071b
valgrind show that probably some mem after kvm_getargv() was uninitialized and used later for g_strlcat(). valgrind happy only after memset()+sysctl(). I do not dig dipper and dont know what happen inside sysctl(). double does not make sense here, and it can consume much more resources, especially on non [86 arch. Some one say that bzero() deprecated, at least in freebsd kernel, so I do not use it. In mine own code I have mem_bzero(), and for other iI prefer memset().
rim referenced this bugreport in commit 5be9587903122389c955f5e4ecc8ff0539542d7b Add G_DEBUG_FMT macro (bug 14401, bug 14403) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=5be9587903122389c955f5e4ecc8ff0539542d7b
rim referenced this bugreport in commit fa797de1406767b38fc709d05a15931142827c56 More sizeof uses (bug 14401, bug 14403) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=fa797de1406767b38fc709d05a15931142827c56
rim referenced this bugreport in commit 704411a38b906e4a70d6cff3afe60d281ef8ebc7 Properly use float for constants where appropriate (bug 14401, bug 14403) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=704411a38b906e4a70d6cff3afe60d281ef8ebc7
rim referenced this bugreport in commit 3af7e3d491e80c283febe0b306a1bb1213f7fd88 Use sysctl w/ KERN_PROC_ARGS instead of kvm_getargv in get_task_details() (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=3af7e3d491e80c283febe0b306a1bb1213f7fd88
I think i pushed all the changes from this bug to master, will look at finishing bug 14403 tomorrow - thanks for this work !
rim referenced this bugreport in commit a76b13057cf8f273ee5f61707b4cf07983f9a6e3 Use sizeof in get_hostname (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=a76b13057cf8f273ee5f61707b4cf07983f9a6e3
Created attachment 7746 less #ifdef code
Created attachment 7748 speedup app-manager, force types of some vars
Created attachment 7749 less #ifdef code
Created attachment 7750 multiple changes - mark unsed vars with '__unused' - remove startup message "Running as %s on %s" and code required only for this: xtm_task_manager_get_username(), xtm_task_manager_get_hostname(), get_owner_uid(), get_hostname() - move getpwuid() from backends to getpwuid_r() in GUI code that add task - linux: 'if ((file = fopen (filename, "r")) == NULL || fgets (buffer, sizeof(buffer), file) == NULL)' - may leak file fd - many type conversions and small improvements
(In reply to Ivan 83 from comment #16) > Created attachment 7750 > multiple changes multiple independent changes, so why not splitting them into separate commits ? Please, this makes it very hard to review otherwise... or push a git repo somewhere so that we can cherry pick selected fixes. why are you switching pid from guint to gint everywyere ? I've applied the two other patches (cf https://git.xfce.org/apps/xfce4-taskmanager/commit/?id=621de4fd13b468189a814aa654572e84c47269fe & https://git.xfce.org/apps/xfce4-taskmanager/commit/?id=8d4f013877b33f22c8cdfe48aaf6a494571b267b)
Created attachment 7751 pif to GPid
Created attachment 7753 pid to miss GPid + one line
Created attachment 7754 force types of some vars, mar vars __unused, move getpwuid() to frontend...
Created attachment 7756 code cleanup: mark unused, remove unused
Created attachment 7757 linux backend fd leak
Created attachment 7758 Move getpwuid() to frontend
Created attachment 7759 Remove unused funcs
Created attachment 7760 type fixes, change formating and micro improvements
rim referenced this bugreport in commit ceb5a7d347b14ed71bf66e60fcb751b8ab868dc6 Make pid type: GPid (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=ceb5a7d347b14ed71bf66e60fcb751b8ab868dc6
rim referenced this bugreport in commit 66a0d4bfc9e13de705b97bbe395b7b7d42a38276 Mark unused params, remove unused macro and args (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=66a0d4bfc9e13de705b97bbe395b7b7d42a38276
rim referenced this bugreport in commit 40a3e65803a87770138d49acf79da33398497f6a Fix possible file descriptor leak on linux (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=40a3e65803a87770138d49acf79da33398497f6a
rim referenced this bugreport in commit 8e1b48f8def94d7b1b72464aaf495d761d46176a Move getpwuid() from backends to gui, use getpwuid_r(), call only once on task add. (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=8e1b48f8def94d7b1b72464aaf495d761d46176a
rim referenced this bugreport in commit 0111851b4d6cdc87699a932b208ebf783491dbde Stop displaying 'Running as <uid> on <hostname>' and remove corresponding code (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=0111851b4d6cdc87699a932b208ebf783491dbde
rim referenced this bugreport in commit a5f4aaa667ad34058219945c195b502de2688cf1 Improve GOptionEntry initialization (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=a5f4aaa667ad34058219945c195b502de2688cf1
rim referenced this bugreport in commit 512e88ad6ca039d001f622e6f5e8f6428e0d30c9 timeout is an unsigned integer (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=512e88ad6ca039d001f622e6f5e8f6428e0d30c9
rim referenced this bugreport in commit 1c09b7329b9df1a7a675f4a03b5261bd4edf8698 Use the proper types, add casts where necessary (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=1c09b7329b9df1a7a675f4a03b5261bd4edf8698
rim referenced this bugreport in commit 9ed5f79f8dbe95a531c1515af7c470c497663c10 More float initializers (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=9ed5f79f8dbe95a531c1515af7c470c497663c10
rim referenced this bugreport in commit e8b4fa49bbb0bba59d5261face8deb63772ff81b xtm_process_tree_model_get_value returns void (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=e8b4fa49bbb0bba59d5261face8deb63772ff81b
rim referenced this bugreport in commit 7de82ec291fe810b88e5cebb877b973e74481c78 Better zeroing of found struct (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=7de82ec291fe810b88e5cebb877b973e74481c78
rim referenced this bugreport in commit 295e635606d7348b22c6969a24a9f6a671dd759e Fix useless indentation (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=295e635606d7348b22c6969a24a9f6a671dd759e
rim referenced this bugreport in commit ccb9824ce2acf69c56774f4a3f36085943f4687d Select_Window returns a static Window (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=ccb9824ce2acf69c56774f4a3f36085943f4687d
rim referenced this bugreport in commit bbee8970794caddd3e7231cd0b05be1a12eda5d4 use a 1k buffer for the initial argv malloc (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=bbee8970794caddd3e7231cd0b05be1a12eda5d4
rim referenced this bugreport in commit 01abcbdd73377b8b416272d6714fabda59cad0a7 Properly bzero args (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=01abcbdd73377b8b416272d6714fabda59cad0a7
rim referenced this bugreport in commit 3d7f9fd794e48e00a5bf5b9b9b2d76d40d943dc7 parenthesis around addition (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=3d7f9fd794e48e00a5bf5b9b9b2d76d40d943dc7
rim referenced this bugreport in commit f51c2b18f7462d9c258ad6b86cf0e02ee4a45d14 Rework the way model is iterated through (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=f51c2b18f7462d9c258ad6b86cf0e02ee4a45d14
rim referenced this bugreport in commit 6cfb7547b0de3cdeffd618b0d804d0a77af3d0aa More cast/type fixes (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=6cfb7547b0de3cdeffd618b0d804d0a77af3d0aa
rim referenced this bugreport in commit 84e719796ec6223dd31107283918c08d22b268cf Simplify the way process state is stored on FreeBSD (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=84e719796ec6223dd31107283918c08d22b268cf
Thanks for the patches - ive applied most of them directly, only rewording the commit msg or adding the bug link, but i had to split https://bugzilla.xfce.org/attachment.cgi?id=7760 into a dozen smaller commits. Having small atomic commits with explicit commit messages really helps when you want to bisect an issue or try to understand the logic behind some changes - so please think to the ones who will read your code later on, be it when reviewing it or trying to understand it. As is your patch mixed lots of non-related stuff, with a really vague commit message, and makes it really hard to review. If you're not going to spend the extra effort justifying your changes (i had a hard time understanding https://git.xfce.org/apps/xfce4-taskmanager/commit?id=f51c2b18f7462d9c258ad6b86cf0e02ee4a45d14) why should i spend 45mn trying to review it ? Patches should be simple to read & understand, and that starts with a clear commit message...
Please pardon my question. bzero/() vs. memset() ? From what I know bzero is considered deprecated and is not a standard c function.
Ok ran anohter compile session of Xfce4 Git tonight and - somehow - I'd expected to get compile errors from all these patches. I'd like to make clear that xfce4-taskmanager git used to compile perfectly before these patches made it into Git. It might be that they improve functionality (and most of them of course are valid) on BSD but they fail compiling on Linux now (Fedora 28). If possible please fix them, so we can compile xfce4-taskmanager git from sources again. Another thing that caught my attention: A long list of different patches *solving different things* applied on one bugreport. For such a big chunk of different patches, a testing branch might come handy. This will prevent the master Git to not break until all issues are sorted out within the branch. Please find attached the build log with the issues I found.
Created attachment 7762 Multiple build issues after all these patches made it into Git master
(In reply to Ali Akcaagac from comment #46) > Please pardon my question. > > bzero/() vs. memset() ? > > From what I know bzero is considered deprecated and is not a standard c > function. At start patches contain memset() only, but after review it was changed to bzero(). See comment 3.
(In reply to Ali Akcaagac from comment #47) > Ok ran anohter compile session of Xfce4 Git tonight and - somehow - I'd > expected to get compile errors from all these patches. > > I'd like to make clear that xfce4-taskmanager git used to compile perfectly > before these patches made it into Git. > > It might be that they improve functionality (and most of them of course are > valid) on BSD but they fail compiling on Linux now (Fedora 28). > > If possible please fix them, so we can compile xfce4-taskmanager git from > sources again. > > Another thing that caught my attention: > > A long list of different patches *solving different things* applied on one > bugreport. For such a big chunk of different patches, a testing branch might > come handy. This will prevent the master Git to not break until all issues > are sorted out within the branch. > > Please find attached the build log with the issues I found. Some how __unsed macro is undefined during build. Check that taskmanager.h contain: #ifndef __unused # define __unused __attribute__((__unused__)) #endif If it there, then probably I should some thing change with it, and I need your compiler name+ver to try reproduce and find workaround.
Yes the headerfile contains the above lines. gcc --version gcc (GCC) 8.1.1 20180502 (Red Hat 8.1.1-1) Copyright (C) 2018 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. The default in Fedora 28 I added #include "task-manager.h" in process-window.c -> compiles I added ... in process-monitor.c -> compiles (the dots mean task-manager.h) I added ... in process-tree-model.c -> compiles Looks like task-manager.h isn't referenced by these three files. $ make and $ make dist works too Program launches...
(In reply to Ivan 83 from comment #49) > At start patches contain memset() only, but after review it was changed to > bzero(). > See comment 3. Interesting. I am not in the position to judge, but from the manuals: http://man7.org/linux/man-pages/man3/bzero.3.html > The bzero() function is deprecated (marked as LEGACY in > POSIX.1-2001); use memset(3) in new programs. POSIX.1-2008 removes > the specification of bzero(). The bzero() function first appeared in > 4.3BSD. For portability reasons and future - possible - termination of bzero, I'd go with memset :) But glibc might have wrapped bzero to memset internally anyways ;)
Landry Breuil referenced this bugreport in commit fcf8beb64eaf6a2dac488ab09cd9b1e911e221ae include task-manager.h to have __unused definition, should fix compilation on linux (bug 14401) https://git.xfce.org/apps/xfce4-taskmanager/commit?id=fcf8beb64eaf6a2dac488ab09cd9b1e911e221ae
I have a personal preference for bzero() because it's shorter to write and simpler. Whatever posix says, i dont have a strong opinion on that. As for the actual build failure, i've added the missing includes you mention in comment #51, does master build for you now ?
(In reply to Landry Breuil from comment #54) > As for the actual build failure, i've added the missing includes you mention > in comment #51, does master build for you now ? I recompiled everything last night and everything went fine. Compiling works Make-Dist works Lauching the Program works Thanks.
Perfect, thanks for the feedback !