Re: [dev] daemon for DWM

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Sun, 21 Jul 2013 15:44:15 +0200

This is an older thread but I just wanted to add some justification
for the code I wrote (which seems to have been used by the author of
this daemon).

On Thu, Jun 27, 2013 at 7:01 PM, Markus Wichmann <nullplan_AT_gmx.net> wrote:
> [...]
>> 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.
>

It looks like this is from the dwmstatus helper function I sent to the
suckless site.

My thought process was that anyone downloading this code should have a
look at the thing he is compiling which would make it obvious that the
functionality is hardcoded to be limited to the wired interface (the
link to the patch has been renamed to reflect this fact though).

I am thinking about considering wlan0 as well and sending a new
version of the code to the suckless site.

>>
>> // ------------------------------------------------------------------------------------------------------
>> 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];

As you probably noticed, I am not very well versed in C so comments
like this are very appreciated and helpful to me.

>> 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.

If I send a new patch I will do that, thanks.

>> 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.

Wouldn't that mean that I would have to save the old value in a
persistent file on disk? Reading and writing the file would add a
significant amount of code whose only advantage it would be to not
have to run this function twice. I do not think that would be worth
it.

>> 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.

I was considering refactoring it but the amount of lines saved would
probably be minimal for a function that is only used twice. If I send
a new version of the patch to the site I will refactor it for
cleanliness sake.

Thanks again for your comments!
Received on Sun Jul 21 2013 - 15:44:15 CEST