[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