On Wed, Jun 26, 2013 at 08:51:46PM -0400, Viola Zoltán wrote:
> Hi, DWM users! I wrote a daemon to the DWM status bar! I send it in
> attachment. The program name "kajjam". Start it simple: ./kajjam
>
Just a few comments on this.
> Compile it with this command:
>
> g++ -funsigned-char -funsigned-bitfields -Wall -Wno-long-long -Wunused
> -Wextra -pedantic -lX11 kajjam.cpp -o kajjam
>
You depend on the signedness of char or bitfields? Really?
> static const char logfilepath[]="/Programs/Kajjam/log/kajjam.log";
> static const char munkakonyvtar[]="/Programs/Kajjam";
> static char idoformatum[]="%m.%d %H:%M ";
And another problem that's been solved: You can just use the locale's
format for a date and time together ("%c") or just a date ("%X") or just
a time ("%x"). The only thing you need to do for that is:
a. Use a libc with full locale support (like glibc. I dislike it
any other time, but here they remain unbeaten)
b. Include <locale.h>
c. At the start of main(), run setlocale(LC_ALL, "");
Also, non-english variable names? Really? I mean, I would understand if
you were at least consistent in that.
> char * smprintf(char *fmt, ...) //
> -------------------------------------------------------------
The function you are looking for is called asprintf().
> while (fgets(buf, bufsize, devfd)) {
> if ((eth0start = strstr(buf, "eth0:")) != NULL) {
^^^^
Oh. Would you mind actually telling people that your code only looks at
eth0? Because I have an eth0 on my laptop, too, but the device doing
basically all my Internet work is called wlan0.
>
> //
> ------------------------------------------------------------------------------------------------------
> char * get_netusage()
> {
> unsigned long long int oldrec, oldsent, newrec, newsent;
> double downspeed, upspeed;
> char *downspeedstr, *upspeedstr;
> char *retstr;
> int retval;
>
> downspeedstr = (char *) malloc(15);
> upspeedstr = (char *) malloc(15);
Those two are freed at the end of the function. Consequently you could
just allocate them automatically:
char downspeedstr[15], upspeedstr[15];
> retstr = (char *) malloc(42);
>
And unless you really want reentrancy and thread-safety, you could
allocate this one statically.
static char retstr[42];
The advantage would be that you don't use malloc(), which can fail, in
places where it is unnecessary.
> retval = parse_netdev(&oldrec, &oldsent);
> if (retval) {
> fprintf(logfilefp, "Error when parsing /proc/net/dev file.\n");
> exit(1);
> }
>
> sleep(1);
> retval = parse_netdev(&newrec, &newsent);
> if (retval) {
> fprintf(logfilefp, "Error when parsing /proc/net/dev file.\n");
> exit(1);
> }
>
You could also save the old values from the previous run, so you would
only need to call parse_netdev() once per run, not twice.
> downspeed = (newrec - oldrec) / 1024.0;
> if (downspeed > 1024.0) {
> downspeed /= 1024.0;
> sprintf(downspeedstr, "%.2f M", downspeed);
> } else {
> // sprintf(downspeedstr, "%.2f K", downspeed);
> sprintf(downspeedstr, "%.f K", downspeed);
> }
>
> upspeed = (newsent - oldsent) / 1024.0;
> if (upspeed > 1024.0) {
> upspeed /= 1024.0;
> sprintf(upspeedstr, "%.2f M", upspeed);
> } else {
> // sprintf(upspeedstr, "%.2f K", upspeed);
> sprintf(upspeedstr, "%.f K", upspeed);
> }
This looks a lot like it could be refactored.
> char * mktimes(char *fmt)
> {
> char buf[129];
> time_t *ido;
> time_t rawtime;
> struct tm ideiglenes;
> time(&rawtime);
> ido=&rawtime;
> ideiglenes = *localtime(ido);
Why that? Why not
time_t tm;
const struct tm* ideiglenes;
tm = time(0);
ideiglenes = localtime(&tm);
It's got one less variable and one less copy of a structure (the
dereference of localtime()'s return value).
> bzero(buf, sizeof(buf));
And this is unneccesary: If strftime succeeds, it will always terminate
the string with a NUL byte. And if not, you won't use the string anyway.
> if (!strftime(buf, sizeof(buf)-1, fmt, &ideiglenes)) {
The -1 here is unneccesary.
> fprintf(logfilefp, "strftime == 0\n");
> exit(1);
> }
> HET[0]=hetnapjai[ideiglenes.tm_wday][0];
> HET[1]=hetnapjai[ideiglenes.tm_wday][1];
> HET[2]=hetnapjai[ideiglenes.tm_wday][2];
> HET[3]=0;
Would strftime()'s "%a" help here?
>
> /* Our process ID and Session ID */
> pid_t pid, sid;
>
> /* Fork off the parent process */
> pid = fork();
> if (pid < 0) {
> exit(EXIT_FAILURE);
> }
> /* If we got a good PID, then
> we can exit the parent process. */
> if (pid > 0) {
> exit(EXIT_SUCCESS);
> }
>
Why do you daemonize here? If people want to start this one in the
background, they can just tell their shell. Also, I wouldn't log to
some hardcoded log file, but rather to stderr. If people want to log
these messages, they can redirect stderr to a log file, and if not, they
can redirect it to /dev/null.
> logfilefp=fopen(logfilepath,"wb+");
> if (!logfilefp) { printf ("Nem tudom megnyitni a %s állományt!\n",
> logfilepath); return(1); }
>
That would also remove this error condition here.
BTW: I just wrote my own time printing daemon:
#include <X11/Xlib.h>
#include <time.h>
#include <locale.h>
#include <unistd.h>
int main(void) {
Display *dpy;
Window root;
setlocale(LC_ALL, "");
dpy = XOpenDisplay(0);
if (dpy) {
int rv;
root = XDefaultRootWindow(dpy);
clock_gettime(CLOCK_REALTIME, &s);
s.tv_sec = 0;
s.tv_nsec = 1000000000 - s.tv_nsec;
do rv = nanosleep(&s, &s);
while (rv == -1);
for (;;) {
char buf[50]; // estimate
time_t t;
size_t l;
t = time(0);
l = strftime(buf, sizeof buf, "%c", localtime(&t));
XStoreName(dpy, root, buf);
XSync(dpy, False);
sleep(1);
}
}
}
Yeah, error handling is pretty much nonexistent. But hey, it does its
job. (The stuff between the XOpenDisplay() and the start of the endless
loop is just there to synchronize by program's clock tick with the
system's, so the output is not on average half a second off.)
Ciao,
Markus
