[PATCH] Support for booting multiple network profiles (netfarm)

Dustin Kirkland dustin.kirkland at us.ibm.com
Sat May 12 20:19:41 EST 2007


On Fri, 2007-05-11 at 16:45 -0300, Paulo Ricardo Paz Vital wrote:
> One scenario we have here, and extremely used, is based in a machine
> booting via network and one TFTP server in the same network. If you want
> to boot via network a machine with a specific config file, you can use
> the netbooting like PXE patch, creating a config file named with the MAC
> adrress of that machine you want boot. 
> 
> One problem in this schema, is that the number of specific config files
> is large if you have a big network infra-structure. Trying to minimize
> the number of different config files provided by TFTP server, we thought
> in a new schema to set the default image entry (option used to load
> automatically a kernel image if no arguments is inputed in yaboot
> prompt) by that label is better for that machine who is booting. 
> 
> How can we do it? Using the MAC address in label entry and checking,
> when yaboot is booted via network, if the MAC address of that machine
> has a same label in the image structure.
> 
> So, the yaboot.conf can be wrote like this:
> 
> # yaboot.conf 
> init-message = "\nNETBOOT\nHit <TAB> for boot options.\n"
> timeout=600
> default=install-rh4
> image=bootp/vmlinuz
>         label=install-rh4
>         initrd=bootp/initrd
>         initrd-size=10240
>         append="devfs=mount,dall init=/linuxrc --"
>         read-only
> image=bootp/vmlinuz-A
>         label=a2-42-10-00-80-04
>         initrd=bootp/initrd-A
>         initrd-size=10240
>         append="devfs=mount,dall init=/linuxrc --"
>         read-only
> image=bootp/vmlinuz-B
>         label=ee-22-60-00-20-04
>         initrd=bootp/initrd-B
>         initrd-size=10240
>         append="devfs=mount,dall init=/linuxrc --"
>         read-only
> image=bootp/vmlinuz-C
>         label=00-14-5e-9c-4e-aa
>         initrd=bootp/initrd-C
>         initrd-size=10240
>         append="devfs=mount init=/linuxrc --"
>         read-only
> 
> When the machine that has the MAC address 00:14:5e:9c:4e:aa boots via
> network, yaboot will set up the default cf_option to label
> 00-14-5e-9c-4e-aa and load the kernel file vmlinuz-C. 
> 
> If a none of these MAC-style labels match with the machine's MAC
> address, the default cf_option will not be modified and will load the
> vmlinuz kernel.
> 
> We've extensively tested these patch on an IBM System p 505 LPAR and IBM
> BladeCenter JS21.
> 
> Thanks.
> Paulo  

Sounds to me like a pretty useful feature, and the configuration file
interface look sane.  I can this this being very valuable in labs with
many PPC-machines.  Some comments inline below.

> diff --git a/include/cfg.h b/include/cfg.h
> index be34e0f..04e45c7 100644
> --- a/include/cfg.h
> +++ b/include/cfg.h
> @@ -28,5 +28,6 @@ extern char*	cfg_get_strg(char *image, char *item);
>  extern int	cfg_get_flag(char *image, char *item);
>  extern void	cfg_print_images(void);
>  extern char*	cfg_get_default(void);
> +extern int cfg_set_default_by_mac (char *mac_addr);

Match the white-spacing for the rest of the function names.

 
>  #endif
> diff --git a/include/prom.h b/include/prom.h
> index e7ee4a9..ea3f7e5 100644
> --- a/include/prom.h
> +++ b/include/prom.h
> @@ -27,6 +27,7 @@
>  #ifndef PROM_H
>  #define PROM_H
>  
> +#include "types.h"
>  #include "stdarg.h"
>  
>  typedef void *prom_handle;
> @@ -118,4 +119,40 @@ extern void prom_pause(void);
>  extern void *call_prom (const char *service, int nargs, int nret, ...);
>  extern void *call_prom_return (const char *service, int nargs, int nret, ...);
>  
> +//Netboot stuffs

I'm not sure about Yaboot's commenting conventions, but // comments are
generally frowned upon.  You're using proper /* ... */ style comments
below, be consistent.

> +
> +/*
> + * "bootp-response" is the property name which is specified in
> + * the recommended practice doc for obp-tftp. However, pmac
> + * provides a "dhcp-response" property and chrp provides a
> + * "bootpreply-packet" property.  The latter appears to begin the
> + * bootp packet at offset 0x2a in the property for some reason.
> + */
> +
> +struct bootp_property_offset {
> +     char *name; /* property name */
> +     int offset; /* offset into property where bootp packet occurs */
> +};
> +
> +static const struct bootp_property_offset bootp_response_properties[] = {
> +     { .name = "bootp-response", .offset = 0 },
> +     { .name = "dhcp-response", .offset = 0 },
> +     { .name = "bootpreply-packet", .offset = 0x2a },
> +};
> +
> +struct bootp_packet {
> +     __u8 op, htype, hlen, hops;
> +     __u32 xid;
> +     __u16 secs, flags;
> +     __u32 ciaddr, yiaddr, siaddr, giaddr;
> +     unsigned char chaddr[16];
> +     unsigned char sname[64];
> +     unsigned char file[128];
> +     /* vendor options go here if we need them */
> +};
> +
> +struct bootp_packet * prom_get_netinfo (void);
> +char * prom_get_mac (struct bootp_packet * packet);
> +char * prom_get_ip (struct bootp_packet * packet);
> +

This is kind of a large patch.  I would have recommended splitting this
patch into a couple of logical pieces.  The stuff above easily have gone
into a "[PATCH 1/4] netfarm yabooting: headers", as it's all header
information.  It's really up to Paul how he wants to read this stuff.
But in my experience, breaking up patches into logical components seems
to speed a long acceptance, as you can more easily identify the parts
that are clearly 'ok', and those that need some discussion and re-work.

>  #endif
> diff --git a/second/cfg.c b/second/cfg.c
> index 56b058a..9039bb9 100644
> --- a/second/cfg.c
> +++ b/second/cfg.c
> @@ -559,6 +559,40 @@ char *cfg_get_default (void)
>       return ret;
>  }
>  
> +// cfg_set_default_by_mac ()
> +// return 1 if the default cf_option was changed to label with the MAC addr
> +// return 0 if not changed

Sadly, there's not a lot of documentation in this file elsewhere to
cross-reference, but see above about generally-preferred /* ... */
comments.

> +int cfg_set_default_by_mac (char *mac_addr)
> +{
> +     CONFIG *walk;
> +     struct IMAGES *tmp;
> +     char * label = NULL;
> +     int haslabel = 0;
> +
> +     // check if there is an image label equal to mac_addr
> +     for (tmp = images; tmp; tmp = tmp->next) {
> +        label = cfg_get_strg_i (tmp->table, "label");
> +        if (!strcmp(label,mac_addr)){
> +            haslabel = 1;
> +        }
> +     }
> +
> +     if (!haslabel)
> +         return 0;
> +     else {
> +         // if there is an image label equal to mac_addr, change the default
> +         //   cf_options to this image label
> +         //for (walk = curr_table; walk->type != cft_end; walk++) {

Looks like you have some artifacts here that should be removed to avoid confusion?

> +         for (walk = cf_options; walk->type != cft_end; walk++) {
> +             if (!strcasecmp(walk->name,"default")) {
> +                 walk->data = mac_addr;
> +                 return 1;
> +             } 
> +         }
> +         return 0;
> +     }
> +}
> +

Ok, I would have made that second part of above into a separate patch
email, titled something like "[PATCH 2/4] netfarm yabooting:
configuration primitives".

>  /*
>   * Local variables:
>   * c-file-style: "k&r" 
> diff --git a/second/prom.c b/second/prom.c
> index 3407e5a..b297d07 100644
> --- a/second/prom.c
> +++ b/second/prom.c
> @@ -655,6 +655,95 @@ prom_pause(void)
>  }
>  
>  /*
> + * prom_get_netinfo()
> + * returns the packet with all needed info for netboot
> + */
> +struct bootp_packet * prom_get_netinfo (void)
> +{
> +     void *bootp_response = NULL;
> +     char *propname;
> +     struct bootp_packet *packet;
> +     int i = 0, size, offset = 0;
> +     prom_handle chosen;
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))
> +
> +     chosen = prom_finddevice("/chosen");
> +     if (chosen < 0) {
> +          DEBUG_F("chosen=%d\n", chosen);
> +      return 0;
> +     }
> +
> +     for (i = 0; i < ARRAY_SIZE(bootp_response_properties); i++) {
> +         propname = bootp_response_properties[i].name;
> +         size = prom_getproplen(chosen, propname);
> +         if (size <= 0)
> +             continue;
> +
> +         DEBUG_F("using /chosen/%s\n", propname);
> +         offset = bootp_response_properties[i].offset;
> +         break;
> +     }
> +
> +     if (size <= 0)
> +         return NULL;
> +
> +     if (sizeof(*packet) > size - offset) {
> +         prom_printf("Malformed %s property?\n", propname);
> +         return NULL;
> +     }
> +
> +     bootp_response = malloc(size);
> +     if (!bootp_response)
> +         return NULL;
> +
> +     if (prom_getprop(chosen, propname, bootp_response, size) < 0)
> +         return NULL;
> +
> +     packet = bootp_response + offset;
> +     return packet;
> +}
> +
> +/*
> + * prom_get_mac()
> + * returns the mac addr of an net card
> + */
> +char * prom_get_mac (struct bootp_packet * packet)
> +{
> +     char * conf_path;
> +     int i;
> +
> +     if (!packet)
> +        return NULL;
> +
> +     conf_path = malloc(packet->hlen * 3 + 1);

Should probably check if this malloc succeeds, or document why you
either have a high degree of confidence why it should succeed, or why
the resulting behavior will not have disastrous results.  Look in
yaboot.c, there are a few malloc() success checks.  Mirror that behavior
here.

> +     sprintf(conf_path, "%02x", packet->chaddr[0]);
> +
> +     for (i = 1; i < packet->hlen; i++) {
> +      char tmp[4];
> +      sprintf(tmp, "-%02x", packet->chaddr[i]);
> +      strcat(conf_path, tmp);
> +     }
> +
> +     return conf_path;
> +}
> +
> +/*
> + * prom_get_ip()
> + * returns the ip addr of an net card
> + */
> +char * prom_get_ip (struct bootp_packet * packet)
> +{
> +     char * conf_path;
> +
> +     if (!packet)
> +        return NULL;
> +
> +     conf_path = malloc(9);
> +     sprintf(conf_path, "%08x", packet->yiaddr);
> +
> +     return conf_path;
> +}
> +/*

Again, I would have broken the above code into "[PATCH 3/4]: netfarm
yabooting: packet processing".

Importantly, that would allow the below patch to be [PATCH 4/4]: netfarm
yabooting: config file parsing".  Most of the stuff above looks pretty
good to me (not that that's necessarily worth much), and it seems to be
relatively low-risk new code.  The remain code below heavily reworks
some very core yaboot code that affects every yaboot user every time.
If there are going to be problems with a patchset like this, this is the
code that will come under heavy scrutiny.

>   * Local variables:
>   * c-file-style: "k&r"
>   * c-basic-offset: 5
> diff --git a/second/yaboot.c b/second/yaboot.c
> index c0027c3..0e196fa 100644
> --- a/second/yaboot.c
> +++ b/second/yaboot.c
> @@ -331,7 +331,6 @@ load_config_file(struct boot_fspec_t *fspec)
>       char *conf_file = NULL, *p;
>       struct boot_file_t file;
>       int sz, opened = 0, result = 0;
> -     char conf_path[512];
>  
>       /* Allocate a buffer for the config file */
>       conf_file = malloc(CONFIG_FILE_MAX);
> @@ -340,19 +339,7 @@ load_config_file(struct boot_fspec_t *fspec)
>  	  goto bail;
>       }
>  
> -     /* Build the path to the file */
> -     if (_machine == _MACH_chrp)
> -         strcpy(conf_path, "/etc/");
> -     else
> -         conf_path[0] = 0;
> -     if (fspec->file && *fspec->file)
> -         strcat(conf_path, fspec->file);
> -     else
> -         strcat(conf_path, CONFIG_FILE_NAME);
> -
> -

It's possible that I'm not understanding your patch, but I'm not seeing
how you'll allow backward compatibility to old-fashion config file
boots...  Am I missing something with the above code being removed?
Where is this support now?

>       /* Open it */
> -     fspec->file = conf_path;
>       result = open_file(fspec, &file);
>       if (result != FILE_ERR_OK) {
>  	  prom_printf("%s:%d,", fspec->dev, fspec->part);
> @@ -381,6 +368,22 @@ load_config_file(struct boot_fspec_t *fspec)
>  	  goto bail;
>       }
>  
> +     // set the default cf_option to label that has the same MAC addr 
> +     // it only works if there is a label with the MAC addr on yaboot.conf
> +     if (prom_get_devtype(fspec->dev) == FILE_DEVICE_NET) {
> +         //  change the variable bellow to get the MAC dinamicaly

s/dinamicaly/dynamically/

> +         char * macaddr = NULL;
> +         int default_mac = 0;
> +
> +         macaddr = prom_get_mac(prom_get_netinfo());
> +         default_mac = cfg_set_default_by_mac(macaddr);
> +         if (default_mac < 1) {
> +            DEBUG_F("Default label was not changed to macaddr label.\n");
> +         } else {
> +            DEBUG_F("Default label was changed to macaddr label.\n");

It would be useful to print that label within the debug statement, perhaps.

> +         }
> +     }
> +
>       DEBUG_F("Config file successfully parsed, %d bytes\n", sz);
>  
>       /* Now, we do the initialisations stored in the config file */
> @@ -443,102 +446,34 @@ bail:
>  }
>  
>  /*
> - * "bootp-response" is the property name which is specified in
> - * the recommended practice doc for obp-tftp. However, pmac
> - * provides a "dhcp-response" property and chrp provides a
> - * "bootpreply-packet" property.  The latter appears to begin the
> - * bootp packet at offset 0x2a in the property for some reason.
> - */
> -
> -struct bootp_property_offset {
> -     char *name; /* property name */
> -     int offset; /* offset into property where bootp packet occurs */
> -};
> -static const struct bootp_property_offset bootp_response_properties[] = {
> -     { .name = "bootp-response", .offset = 0 },
> -     { .name = "dhcp-response", .offset = 0 },
> -     { .name = "bootpreply-packet", .offset = 0x2a },
> -};
> -
> -struct bootp_packet {
> -     u8 op, htype, hlen, hops;
> -     u32 xid;
> -     u16 secs, flags;
> -     u32 ciaddr, yiaddr, siaddr, giaddr;
> -     unsigned char chaddr[16];
> -     unsigned char sname[64];
> -     unsigned char file[128];
> -     /* vendor options go here if we need them */
> -};
> -

Ok, I see that your header patch actually moves this information.  Both
the - and + lines would go into that [PATCH 1/4] that I suggested if you
got that route.  Which would truly make 4/4 dependent on 1/4.  Either
handle the ordering yourself very carefully, or preferably use quilt.

Lastly, there are a lot of changes below.  In the intro paragraphs of
Patch 4/4, you should probably talk a little be about what you're doing
down here below.  

> -/*
>   * Search for config file by MAC address, then by IP address.
>   * Basically copying pxelinux's algorithm.
>   * http://syslinux.zytor.com/pxe.php#config
>   */
>  static int load_my_config_file(struct boot_fspec_t *orig_fspec)
>  {
> -     void *bootp_response = NULL;
> -     char *propname;
>       struct bootp_packet *packet;
> -     int i = 0, size, offset = 0, rc = 0;
> -     prom_handle chosen;
> +     int rc = 0;
>       struct boot_fspec_t fspec = *orig_fspec;
>  #define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))
>  
> -     chosen = prom_finddevice("/chosen");
> -     if (chosen < 0) {
> -          prom_printf("chosen=%d\n", chosen);
> -	  return 0;
> -     }
> -
> -     for (i = 0; i < ARRAY_SIZE(bootp_response_properties); i++) {
> -	  propname = bootp_response_properties[i].name;
> -	  size = prom_getproplen(chosen, propname);
> -	  if (size <= 0)
> -	       continue;
> -
> -	  DEBUG_F("using /chosen/%s\n", propname);
> -	  offset = bootp_response_properties[i].offset;
> -	  break;
> -     }
> -
> -     if (size <= 0)
> -	  goto out;
> -
> -     if (sizeof(*packet) > size - offset) {
> -	  prom_printf("Malformed %s property?\n", propname);
> -	  goto out;
> -     }
> -
> -     bootp_response = malloc(size);
> -     if (!bootp_response)
> -	  goto out;
> -
> -     if (prom_getprop(chosen, propname, bootp_response, size) < 0)
> -	  goto out;
> -
> -     packet = bootp_response + offset;
> +     packet = prom_get_netinfo();
>  
>       /*
>        * First, try to match on mac address with the hardware type
>        * prepended.
>        */
>  
> -     /* 3 chars per byte in chaddr + 2 chars for htype + \0 */
> -     fspec.file = malloc(packet->hlen * 3 + 2 + 1);
> +     /* 3 chars per byte in chaddr + 2 chars for htype + /etc/ +\0 */
> +     fspec.file = malloc(packet->hlen * 3 + 2 + 6);
>       if (!fspec.file)
>  	  goto out;
>  
> -     sprintf(fspec.file, "%02x", packet->htype);
> -
> -     for (i = 0; i < packet->hlen; i++) {
> -	  char tmp[4];
> -	  sprintf(tmp, "-%02x", packet->chaddr[i]);
> -	  strcat(fspec.file, tmp);
> -     }
> -
> -     //DEBUG_F("----> mac addr: %s\n", fspec.file);
> +     if (_machine == _MACH_chrp)
> +         sprintf(fspec.file, "/etc/%02x-", packet->htype);
> +     else
> +         sprintf(fspec.file, "%02x-", packet->htype);
> +	 strcat(fspec.file, prom_get_mac(packet));
>  
>       rc = load_config_file(&fspec);
>       if (rc)
> @@ -550,7 +485,7 @@ static int load_my_config_file(struct boot_fspec_t *orig_fspec)
>        */
>       free(fspec.file);
>       fspec.file = malloc(9);

Malloc check would be nice (or note that this is very small, should succeed).

> -     sprintf(fspec.file, "%08X", packet->yiaddr);
> +     strcat(fspec.file, prom_get_ip(packet));
>  
>       while (strlen(fspec.file)) {
>  	  rc = load_config_file(&fspec);
> @@ -565,7 +500,6 @@ static int load_my_config_file(struct boot_fspec_t *orig_fspec)
>  	  orig_fspec->file = fspec.file;
>       else
>  	  free(fspec.file);
> -     free(bootp_response);
>       return rc;
>  }


:-Dustin




More information about the Yaboot-devel mailing list