On 19.04.2018 10:02, Viktor VM Mihajlovski wrote:
> On 18.04.2018 14:31, Thomas Huth wrote:
>> The .INS config files can normally be found on CD-ROM ISO images,
>> so by supporting these files, it is now possible to boot directly
>> when the TFTP server is set up with the contents of such an CD-ROM
>> image.
>>
>> Signed-off-by: Thomas Huth <th...@redhat.com>
>> ---
>> pc-bios/s390-ccw/netmain.c | 51
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>>
>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>> index fa62bfe..e52e17d 100644
>> --- a/pc-bios/s390-ccw/netmain.c
>> +++ b/pc-bios/s390-ccw/netmain.c
>> @@ -441,6 +441,55 @@ static int net_try_pxelinux_cfgs(filename_ip_t *fn_ip)
>> return -1;
>> }
>>
>> +/**
>> + * Load via information from a .INS file (which can be found on CD-ROMs
>> + * for example)
>> + */
>> +static int handle_ins_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
>> +{
>> + char *ptr;
>> + int rc = -1, llen;
>> + void *destaddr;
>> + char *insbuf = cfg;
>> +
>> + ptr = strchr(insbuf, '\n');
>> + if (!ptr) {
>> + puts("Does not seem to be a valid .INS file");
>> + return -1;
>> + }
>> +
>> + *ptr = 0;
>> + printf("\nParsing .INS file:\n %s\n", &insbuf[2]);
>> +
>> + insbuf = ptr + 1;
>> + while (*insbuf && insbuf < cfg + cfgsize) {
>> + ptr = strchr(insbuf, '\n');
>> + if (ptr) {
>> + *ptr = 0;
>> + }
>> + llen = strlen(insbuf);
>> + if (!llen) {
>> + insbuf = ptr + 1;
>> + continue;
>> + }
>> + ptr = strchr(insbuf, ' ');
>> + if (!ptr) {
>> + puts("Missing space separator in .INS file");
>> + return -1;
>> + }
>> + *ptr = 0;
>> + strncpy((char *)fn_ip->filename, insbuf, sizeof(fn_ip->filename));
>> + destaddr = (char *)atol(ptr + 1);
>> + rc = tftp_load(fn_ip, destaddr, (long)_start - (long)destaddr);
>> + if (rc <= 0) {
>> + break;
>> + }
>> + insbuf += llen + 1;
>> + }
>> +
>> + return rc;
>> +}
>> +
>> static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>> {
>> int rc;
>> @@ -455,6 +504,8 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>> if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf,
>> 2)) {
>> /* Looks like it is a pxelinux.cfg */
>> return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
>> + } else if (!strncmp("* ", cfgbuf, 2)) {
>> + return handle_ins_cfg(fn_ip, cfgbuf, rc);
>> }
>> }
>>
> You could consider to move the magic matching code into a helper
> function, I could imagine that the detection might grow more complex
> over time and then could clutter the try-load code.
> I.e. something like
>
> typedef enum {
> FT_UNKOWN,
> FT_PXECFG,
> FT_INSFILE,
> } FileType;
>
> static FileType probe_file_type(const char * cfgbuf, size_t cfgbuflen)
> {
> ...
> if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ",
> cfgbuf, 2)) {
> /* Looks like it is a pxelinux.cfg */
> return FT_PXECFG;
> } else if (!strncmp("* ", cfgbuf, 2)) {
> return FT_INSFILE;
> } else {
> return FT_UNKOWN;
> }
> }
>
> ...
> FileType ft = probe_file_type(cfgbuf, ...);
> switch (ft)
> {
> case FT_PXECFG:
> return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
> break;
> ...
> case FT_UNKNOWN:
> /* got to be a kernel */
> memmove(...);
> return rc;
> }
> ...
I agree that this might be a good refactoring if the list grows bigger,
but as long as we only have these few entries here, it looks a little
bit over-engineered to me, so I'd rather prefer to keep the code in its
current shape right now.
Thomas
