On Friday 08 February 2008 14:20, Nikos Mavrogiannopoulos wrote:
> On Tuesday 05 February 2008 15:18:36 Nikos Mavrogiannopoulos wrote:
> 
> > > > I've submitted a patch in http://bugs.busybox.net/view.php?id=1999 to
> > > > support the DHCP options 121, 63. The static routes option exports a
> > > > variable $sroutes that contains NETWORK1 NETMASK1 GATEWAY1 NETWORK2
> > > > NETMASK2 GATEWAY2 etc. The vendorinfo option is quite intrusive and
> > > > allows to export vendorinfo suboptions via the dhcp_sub_options
> > > > structure.
> > >
> > > Needs to be rediffed against 1.9.0. There were many changes in DHCP
> > > applets in 1.9.0.
> > I have uploaded a patch for the svn. However I have not tested it yet.
> 
> It seems my last patch wasn't working. I've reviewed this patch and updated 
> for the new udhcp interface (though I think the previous was a bit more 
> straightforward),

Sorry about that. Maybe it was a bit simpler, but new one is smaller.

> and uploaded it at: 
> http://bugs.busybox.net/file_download.php?file_id=1504&type=bug

+static uint8_t* mask_to_ip(uint8_t * ip, uint8_t mask)
+{
+int i;
+uint32_t ret = 0;

Broken indent

+
+       for (i=0;i<mask;i++)
+               ret |= (1 << i);
+                                    <== stray space chars here
+       ip[3] = (ret >> 24) & 0xff;

There are other lines which are empty except for many spaces.
Delete those spaces.

+inline
+static void extract_route_octets( uint8_t *ret_ip, uint8_t* p, int mask, int 
*adv)

It should not be inline (too big).
Instead of using int *adv parameter, just advance and return new p
as a return value of th function.

mask should be 'unsigned' (I want to do (mask / 8), and signed / 8
cannot be done with shifts).

+{
+       *adv = 0;
+
+       if (mask >= 25 && mask <= 32) {
+               *adv = 4;
+               ret_ip[0] = p[0];
+               ret_ip[1] = p[1];
+               ret_ip[2] = p[2];
+               ret_ip[3] = p[3];
+
+               return;
+       } else if (mask >= 17 && mask <= 24) {
+               *adv = 3;
+               ret_ip[0] = p[0];
+               ret_ip[1] = p[1];
+               ret_ip[2] = p[2];
+               ret_ip[3] = 0;
+
+               return;
+       } else if (mask >= 9 && mask <= 16) {
+               *adv = 2;
+               ret_ip[0] = p[0];
+               ret_ip[1] = p[1];
+               ret_ip[2] = 0;
+               ret_ip[3] = 0;
+
+               return;
+       } else if (mask >= 1 && mask <= 8) {
+               (*adv) = 1;
+               ret_ip[0] = p[0];
+               ret_ip[1] = 0;
+               ret_ip[2] = 0;
+               ret_ip[3] = 0;
+
+               return;
+       }
+       ret_ip[0] = ret_ip[1] = ret_ip[2] = ret_ip[3] = 0;
+}

Bloat! How about this?

{
        memset(ret_ip, 0, 4);
        if (mask <= 32) {
                mask = (mask+7) / 8;  /* 1..8 -> 1, 9..16 -> 2 etc */
                while (mask--)
                        *ret_ip++ = *p++;
        }
        return p;
}


+               case OPTION_SUBOPTIONS: {
+                       struct dhcpMessage sub_msg;
+                       uint8_t* temp;
+                       int i;

Broken indentation again.

+                               memset( &sub_msg, 0, sizeof(sub_msg));
+                               memcpy( sub_msg.options, option, MIN(len, 
sizeof(sub_msg.options)));

Please keep style consistent. memset(a, b, c), not memset( a, b, c).

+                               for (i = 0; dhcp_sub_options[i].code; i++) {

Loop? Why? dhcp_sub_options has only one element.


+               case OPTION_STATIC_ROUTES: {
+                       /* we return the static routes in a string containing a 
list
+                        * of NETWORK1 NETMASK1 GATEWAY1 NETWORK2 NETMASK2 
GATEWAY2 ... */
+                           int left = len;

Suddenly, you started indenting with four spaces here, not one tab.

+                           int mask;
+                           int adv;
+                           uint8_t *p = option;
+                           uint8_t ip[4];
+
+                           *dest = 0;
+
+                           do {
+                               mask = *p;
+                               p++;
+                               left -= 1;
+                               if (left <= 0) break;
+
+                               extract_route_octets( ip, p, mask, &adv);
+                               p += adv;
+                               left -= adv;
+                               if (left <= 0) break;
+
+                               /* print ip */
+                               dest += sprintip(dest, "", ip);
+                               *dest++ = ' ';
+
+                               dest += sprintip(dest, "", mask_to_ip(ip, 
mask));
+                               *dest++ = ' ';
+
+                               /* print router */
+                               dest += sprintip(dest, "", p);
+                               *dest++ = ' ';
+                               p += 4;
+                               left -= 4;

Here:

+                               if (left < 0) break;   <======= not needed
+                           } while(left > 0);


+ /* gennet additions */
+       { OPTION_STATIC_ROUTES|OPTION_REQ,        DHCP_STATIC_ROUTES}, /* 
sroutes */
+       { OPTION_SUBOPTIONS|OPTION_REQ,           DHCP_VENDOR_INFO},   /* 
vendorinfo */

I don't this these options should be requested by default.
Please remove OPTION_REQ bit.

Looking forward for next iteration of the patch.
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox

Reply via email to