[PATCH] ifplugd: simple code shrink (~ -43)

Denys Vlasenko vda.linux at googlemail.com
Sat Jul 10 18:38:32 UTC 2010


On Sat, Jul 10, 2010 at 4:11 PM, Maksym Kryzhanovskyy <xmaks at email.cz> wrote:
> Denys Vlasenko wrote:
>> Hi,
>>
>> On Friday 09 July 2010 13:41, Maksym Kryzhanovskyy wrote:
>> > attached is a patch for ifplugd. Please review and consider usage.
>>
>> Max, please briefly explain what this patch fixes.
>>
>> And, if there are both cosmetic changes and logic changes,
>> it makes sense to split the patch into two - it's easier
>> to understand the changes that way.
>>
>
> I split the previous patch into two, logical and cosmetic.
>
> The first patch simplifies the method choice for detecting the link state,
> also it makes 'set_ifreq_to_ifname' function more generic:

-static void set_ifreq_to_ifname(struct ifreq *ifreq)
+static void set_ifreq_to_ifname(void *req, size_t sz)
 {
-       memset(ifreq, 0, sizeof(struct ifreq));
-       strncpy_IFNAMSIZ(ifreq->ifr_name, G.iface);
+       struct req {
+               union {
+                       char ifname[IFNAMSIZ];
+               } u;
+       };
+       memset(req, 0, sz);
+       strncpy_IFNAMSIZ(((struct req *)req)->u.ifname, G.iface);
 }

What's the point of adding the sz parameter, if it is always
sizeof(struct ifreq) in every callsite?


> function                                             old     new   delta
> ifplugd_main                                        1056    1119     +63
> maybe_up_new_iface                                    33      35      +2
> detect_link                                          216     208      -8
> detect_link_wlan                                     120      99     -21
> set_ifreq_to_ifname                                   32       -     -32
> static.method                                         40       -     -40
> .rodata                                           133272  133232     -40
> ------------------------------------------------------------------------------
> (add/remove: 0/2 grow/shrink: 2/3 up/down: 65/-141)           Total: -76 bytes

I see this instead:

function                                             old     new   delta
ifplugd_main                                        1055    1130     +75
up_iface                                             112     117      +5
detect_link_priv                                      80      85      +5
detect_link_mii                                       80      85      +5
detect_link_iff                                       71      76      +5
detect_link_ethtool                                   65      70      +5
maybe_up_new_iface                                    33      36      +3
set_ifreq_to_ifname                                   36      33      -3
detect_link                                          217     214      -3
detect_link_wlan                                     125     105     -20
static.method                                         40       -     -40
------------------------------------------------------------------------------
(add/remove: 0/1 grow/shrink: 7/3 up/down: 103/-66)            Total: 37 bytes

and with set_ifreq_to_ifname change rolled back, it is still
getting bigger, not smaller:

ifplugd_main                                        1055    1125     +70
maybe_up_new_iface                                    33      36      +3
detect_link                                          217     214      -3
static.method                                         40       -     -40
------------------------------------------------------------------------------
(add/remove: 0/1 grow/shrink: 2/1 up/down: 73/-43)             Total: 30 bytes
   text	   data	    bss	    dec	    hex	filename
 837992	   7528	      0	 845520	  ce6d0	busybox_old
 838022	   7528	      0	 845550	  ce6ee	busybox_unstripped


> The second patch includes rename the named constants acording to bb convention
> and removes those which are not used. No size changes.

The second patch should include only new changes, not all changes combined.
Otherwise it's harder to review.

-- 
vda


More information about the busybox mailing list