[PATCH] Support for booting multiple network profiles (netfarm)
Paulo Ricardo Paz Vital
vital at br.ibm.com
Tue May 15 00:03:50 EST 2007
Dustin,
I really appreciated your comments. I think that this kind of feedback
is very important to us (me and community), either to discuss the best
way to code Yaboot, or to learn!!
Some comments about your comments inline bellow.
On Sat, 2007-05-12 at 05:19 -0500, Dustin Kirkland wrote:
> 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.
It is very nice to know that you liked this patch.
> > 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.
Well, I've never read (because I've never found) any document about
Yaboot's programming conventions. I, particulary use the // comments
when I wrote 1,2 or 3 lines. But I'll modify this style comments to that
more used in the code.
Some parts of my patch are only reallocation of netbooting like PXE
patch (Guillon's patch). To this patch it's important get the MAC
address of that machine is booting, and Guillon's patch do it. So, to
use the same algorithm (and don't reinvent the wheel), I carried the
code to another space. The /* ... */ style comments are not mine!
> > +
> > +/*
> > + * "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.
I agree with you that this is a large patch, and for my future patch
submissions I'll split the large patches into a couple pieces.
> > #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?
Oops! I forgot to delete the last // line. :-D
>
> > + 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".
Got it!!
> > /*
> > * 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.
Actually, this malloc is a modified part of netbooting like PXE code. I
forgot to keep the comment used to explain why it succeed. I'll check
the yaboot.c code and modify this part to check if the malloc succeeds.
> > + 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?
This part is a correction of my last patch submitted. I sent this part
to Paul a few weeks ago. But when I used git to generate this patch,
this part was included.
> > /* 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/
Oops!!
> > + 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.
Hhuumm!!! That is a nice suggestion! I'll modify my patch to print the
label.
> > + }
> > + }
> > +
> > 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).
I'll rewrite the malloc parts!
> > - 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
Dustin, your comments were helpful and I'll submit this patch with your
suggestions.
Best regards,
Paulo Vital
"Living and learning!" - my father
More information about the Yaboot-devel
mailing list