> index 47ac6ae..f6999e3 100644
> --- a/include/fw_context.h
> +++ b/include/fw_context.h
> @@ -33,8 +33,9 @@ struct boot_context {
>       char chap_password_in[16];
>       char iface[42];
>       char mac[18];
> -     char ipaddr[18];
> -     char mask[18];
> +     char ipaddr[32];
> +     char mask[19];
> +     char gwaddr[32];

Why these changes? Is expanding the length of the IP to 32 for the purpose
of handling IPv6? If that is the case, where is the code for that? 

Why make the mask 19 bytes? 16 looks like the correct value, except if you
want the netmask to support IPv6 at which point I would have thought
32 would be the better number?

> diff --git a/utils/fwparam_ibft/fwparam_ibft.c 
> b/utils/fwparam_ibft/fwparam_ibft.c
> index d6b8b7f..f800191 100644
> --- a/utils/fwparam_ibft/fwparam_ibft.c
> +++ b/utils/fwparam_ibft/fwparam_ibft.c
> @@ -81,8 +81,11 @@ format_lun(char *buf, size_t size, uint8_t *lun)
>  {
>       int i;
>  
> -     for (i = 0; i < 8; i++)
> -             snprintf(buf++, size--, "%x", lun[i]);

Why not just change that to %08x ?


> +     for (i = 0; i < 8; i++) {
> +             snprintf(buf, size, "%02x", lun[i]);
> +             buf += 2;
> +             size -= 2;
> +     }
>  }
>  
>  void
> @@ -147,6 +150,32 @@ format_ipaddr(char *buf, size_t size, uint8_t *ip)
>  
>  }
>  
> +void
> +format_netmask(char *buf, size_t size, uint8_t mask)
> +{
> +     uint32_t tmp;
> +
> +     tmp = 0xffffffff << (32 - mask);
> +     sprintf(buf,"%02x.%02x.%02x.%02x",
> +             (tmp >> 24) & 0xff,
> +             (tmp >> 16) & 0xff,
> +             (tmp >>  8) & 0xff,
> +             tmp & 0xff);
> +}
> +
> +void
> +format_mac(char *buf, size_t size, uint8_t *mac)
> +{
> +     int i;
> +
> +     for (i = 0; i < 5; i++) {
> +             sprintf(buf, "%02x:", mac[i]);
> +             buf += 3;
> +     }
> +     sprintf(buf, "%02x", mac[i]);

Uh, why not:
         sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
                               (u8)mac[0], (u8)mac[1], (u8)mac[2],
                               (u8)mac[3], (u8)mac[4], (u8)mac[5]);

instead of this loop?
> +}
> +
> +
>  /*
>   * Dump the 16 byte ipaddr, as IPV6 or IPV4.
>   */
> @@ -316,7 +345,7 @@ dump_ibft(void *ibft_loc, struct boot_context *context)
>       struct ibft_initiator *initiator = NULL;
>       struct ibft_nic *nic0 = NULL, *nic1 = NULL;
>       struct ibft_tgt *tgt0 = NULL, *tgt1 = NULL;
> -     char ipbuf[32];
> +     char buf[32];
>  
>       control = ibft_loc + sizeof(*ibft_hdr);
>       CHECK_HDR(control, control);
> @@ -356,18 +385,79 @@ dump_ibft(void *ibft_loc, struct boot_context *context)
>               CHECK_HDR(tgt1, target);
>       }
>  
> +     if (!context) {
> +             snprintf(buf, sizeof(buf), "iSCSI_INITIATOR_");
> +
> +             if (initiator && (initiator->hdr.flags &
> +                               INIT_FLAG_FW_SEL_BOOT))
> +                     dump_initiator_prefix(ibft_loc, initiator, buf);
> +
> +             if (nic0 && (nic0->hdr.flags & INIT_FLAG_FW_SEL_BOOT))
> +                     dump_nic_prefix(ibft_loc, nic0, buf);
> +             else if (nic1 && (nic1->hdr.flags & INIT_FLAG_FW_SEL_BOOT))
> +                     dump_nic_prefix(ibft_loc, nic1, buf);
> +
> +             snprintf(buf, sizeof(buf), "iSCSI_TARGET_");
> +
> +             if (tgt0 && (tgt0->hdr.flags & INIT_FLAG_FW_SEL_BOOT))
> +                     dump_tgt_prefix(ibft_loc, tgt0, buf);
> +             else if (tgt1 && (tgt1->hdr.flags & INIT_FLAG_FW_SEL_BOOT))
> +                     dump_tgt_prefix(ibft_loc, tgt1, buf);
> +
> +             return 0;
> +     }
> +
>       strncpy(context->initiatorname,
>               (char *)ibft_loc+initiator->initiator_name_off,
>               initiator->initiator_name_len + 1);
>  
> +     if (nic0 && (nic0->hdr.flags & INIT_FLAG_FW_SEL_BOOT)) {
> +             format_ipaddr(buf, sizeof(buf),
> +                           nic0->ip_addr);
> +             strcpy((char *)context->ipaddr, buf);
> +
> +             format_ipaddr(buf, sizeof(buf),
> +                           nic0->gateway);
> +             strcpy((char *)context->gwaddr, buf);
> +
> +             format_mac(buf, sizeof(buf),
> +                        nic0->mac);
> +             strcpy((char *)context->mac, buf);
> +
> +             format_netmask(buf, sizeof(buf),
> +                            nic0->subnet_mask_prefix);
> +             strcpy((char *)context->mask, buf);
> +     }
> +
> +     if (nic1 && (nic1->hdr.flags & INIT_FLAG_FW_SEL_BOOT)) {
> +             format_ipaddr(buf, sizeof(buf),
> +                           nic1->ip_addr);
> +             strncpy((char *)context->ipaddr, buf,
> +                     sizeof(buf));
> +             format_ipaddr(buf, sizeof(buf),
> +                           nic1->gateway);
> +             strncpy((char *)context->gwaddr, buf,
> +                     sizeof(buf));
> +
> +             format_mac(buf, sizeof(buf),
> +                        nic1->mac);
> +             strncpy((char *)context->mac, buf,
> +                     sizeof(buf));

That looks a like a buffer overflow. The context->mac is 18 bytes
while the buf is length 32. Granted latter on we overwrite it, but
nonethless...

> +
> +             format_netmask(buf, sizeof(buf),
> +                            nic1->subnet_mask_prefix);
> +             strncpy((char *)context->mask, buf,
> +                     sizeof(buf));
> +     }
> +

All of those strncpy look wrong. I would think we want to do
sizeof(context->[x]) instead of sizeof(buf).

Now I understand why you made the values 32 bytes long but I would
think the proper way is to keep them the right size (17 bytes - and then
expand them to the right size when IPv6 is included here) and just copy
the right length to the fields.

>       if (tgt0 && (tgt0->hdr.flags & INIT_FLAG_FW_SEL_BOOT)) {
>               strncpy((char *)context->targetname,
>                       (char *)(ibft_loc+tgt0->tgt_name_off),
>                       tgt0->tgt_name_len);
> -             format_ipaddr(ipbuf, sizeof(ipbuf),
> +             format_ipaddr(buf, sizeof(buf),
>                             tgt0->ip_addr);
> -             strncpy((char *)context->target_ipaddr, ipbuf,
> -                     sizeof(ipbuf));
> +             strncpy((char *)context->target_ipaddr, buf,
> +                     sizeof(buf));
>               context->target_port = tgt0->port;
>               strncpy(context->chap_name,
>                       (char *)(ibft_loc + tgt0->chap_name_off),
> @@ -386,10 +476,10 @@ dump_ibft(void *ibft_loc, struct boot_context *context)
>               strncpy((char *)context->targetname,
>                       (char *)(ibft_loc+tgt1->tgt_name_off),
>                       tgt1->tgt_name_len);
> -             format_ipaddr(ipbuf, sizeof(ipbuf),
> +             format_ipaddr(buf, sizeof(buf),
>                             tgt1->ip_addr);
> -             strncpy((char *)context->target_ipaddr,ipbuf,
> -                     sizeof(ipbuf));
> +             strncpy((char *)context->target_ipaddr,buf,
> +                     sizeof(buf));
>               context->target_port = tgt1->port;
>               strncpy(context->chap_name,
>                       (char *)(ibft_loc + tgt1->chap_name_off),
> @@ -523,7 +613,7 @@ fwparam_ibft(struct boot_context *context, const char 
> *filepath)
>       if (ibft_loc)
>               ret = dump_ibft(ibft_loc, context);
>       else {
> -             printf("Could not find iBFT.\n");
> +             fprintf(stderr, "Could not find iBFT.\n");
>               ret = -1;
>       }
>       munmap(filebuf, end_search);
> diff --git a/utils/fwparam_ibft/fwparam_main.c 
> b/utils/fwparam_ibft/fwparam_main.c
> new file mode 100644
> index 0000000..95a1a4d
> --- /dev/null
> +++ b/utils/fwparam_ibft/fwparam_main.c
> @@ -0,0 +1,121 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
> + * USA.
> + *
> + * Copyright (C) IBM Corporation, 2006
> + *
> + * Authors:  Patrick Mansfield <[EMAIL PROTECTED]>
> + *           Mike Anderson   <[EMAIL PROTECTED]>
> + *           Hannes Reinecke <[EMAIL PROTECTED]>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <dirent.h>
> +
> +#include "fwparam_ibft.h"
> +#include "fw_context.h"
> +
> +extern int debug;
> +
> +int get_ifnum_from_mac(char *mac)
> +{
> +     int ifnum = -1, fd;
> +     DIR *d;
> +     struct dirent *dent;
> +     char buf[20], attr[64];
> +
> +     d = opendir("/sys/class/net");
> +     while ((dent = readdir(d))) {
> +             if (dent->d_name[0] == '.')
> +                     continue;
> +
> +             sprintf(attr,"/sys/class/net/%s/address", dent->d_name);
> +             fd = open(attr,O_RDONLY);
> +             if (!fd)
> +                     continue;
> +
> +             read(fd, buf, 18);

Why make the buf 20 if we just read 18 in?

> +             close(fd);
> +
> +             if (strncmp(mac, buf, strlen(mac)))
> +                     continue;
> +
> +             if (sscanf(dent->d_name,"eth%d", &ifnum) == 1)
> +                     break;
> +     }
> +     closedir(d);
> +
> +     return ifnum;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +     int option, ret, do_ipconfig = 0;
> +     char *progname, *filebuf = NULL;
> +     struct boot_context ctxt;
> +
> +     progname = argv[0];
> +
> +     while (1) {
> +             option = getopt(argc, argv, "f:ivhb");
> +             if (option == -1)
> +                     break;
> +             switch (option) {
> +             case 'b':
> +                     /* Ignored for compability */
> +                     break;
> +             case 'f':
> +                     filebuf = optarg;
> +                     break;
> +             case 'i':
> +                     do_ipconfig = 1;
> +                     break;
> +             case 'v':
> +                     debug++;
> +                     break;
> +             default:
> +                     fprintf(stderr, "Unknown or bad option '%c'\n", option);
> +             case 'h':
> +                     printf("Usage: %s OPTIONS\n"
> +                            "-b print only fw boot selected sections\n"
> +                            "-f file_to_search (default /dev/mem)\n"
> +                            "-v verbose\n",
> +                            progname);
> +                     exit(1);
> +             }
> +     }
> +
> +     if (!do_ipconfig)
> +             ret = fwparam_ibft(NULL, filebuf);
> +     else {
> +             ret = fwparam_ibft(&ctxt, filebuf);
> +             if (!ret)
> +                     /*
> +                      * Format is:
> +                      * ipaddr:peeraddr:gwaddr:mask:hostname:iface:none

So that won't be the case if the ipaddr comes out to be a DHCP address. The spec
says that the DHCP address is when all 16 bytes are of zeros. 'format_ipaddr' 
doesn't
check for that and makes it '\0'. So your code would print out (per the code 
below
since the format above is incorrect (you don't get the hostname from anywhere).

:::::eth0:ibft


> +                      */
> +                     printf("%s::%s:%s::eth%d:ibft\n",

That looks as if there are too many ":"? Or is the format different? Why aren't 
there
a 'none'?

> +                            ctxt.ipaddr, ctxt.gwaddr,
> +                            ctxt.mask, get_ifnum_from_mac(ctxt.mac));

> +     }
> +     exit(ret);
> +}
> -- 
> 1.5.3.4
> 
> 
> 

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---

Reply via email to