Re: [Qemu-devel] [PATCH 2/3] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Viktor VM Mihajlovski <mihajlov@linux.vnet.ibm.com>]

On 30.05.2018 11:16, Thomas Huth wrote:
> Since it is quite cumbersome to manually create a combined kernel with
> initrd image for network booting, we now support loading via pxelinux
> configuration files, too. In these files, the kernel, initrd and command
> line parameters can be specified seperately, and the firmware then takes
> care of glueing everything together in memory after the files have been
> downloaded. See this URL for details about the config file layout:
> https://www.syslinux.org/wiki/index.php?title=PXELINUX
> 
> The user can either specify a config file directly as bootfile via DHCP
> (but in this case, the file has to start either with "default" or a "#"
> comment so we can distinguish it from binary kernels), or a folder (i.e.
> the bootfile name must end with "/") where the firmware should look for
> the typical pxelinux.cfg file names, e.g. based on MAC or IP address.
> We also support the pxelinux.cfg DHCP options 209 and 210 from RFC 5071.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/netboot.mak |  7 ++--
>  pc-bios/s390-ccw/netmain.c   | 79 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 4 deletions(-)
[...]
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index 7533cf7..e84bb2b 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
[...]
> @@ -301,6 +363,18 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>          if (!strncmp("* ", cfgbuf, 2)) {
>              return handle_ins_cfg(fn_ip, cfgbuf, rc);
>          }
> +        if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf, 2)) {
Minor, but I'm wondering whether this is not too cautious and could rule
out valid config files. You might just unconditionally call
pxelinux_parse_cfg and let it find out if this is as pxelinux config
file or not.
> +            /* Looks like it is a pxelinux.cfg */
> +            struct pl_cfg_entry entries[MAX_PXELINUX_ENTRIES];
> +            int num_ent, def_ent = 0;
> +
> +            num_ent = pxelinux_parse_cfg(cfgbuf, sizeof(cfgbuf), entries,
> +                                         MAX_PXELINUX_ENTRIES, &def_ent);
> +            if (num_ent <= 0) {
> +                return -1;
> +            }
> +            return load_kernel_with_initrd(fn_ip, &entries[def_ent]);
> +        }>      }
> 
>      /* Move kernel to right location */
> @@ -406,6 +480,9 @@ void main(void)
>      if (fnlen > 0 && fn_ip.filename[fnlen - 1] != '/') {
>          rc = net_try_direct_tftp_load(&fn_ip);
>      }
> +    if (rc <= 0) {
> +        rc = net_try_pxelinux_cfg(&fn_ip);
> +    }
> 
>      net_release(&fn_ip);
> 


-- 
Regards,
  Viktor Mihajlovski