Re: [PATCH] PXE support for grub2

[Bean]

On Tue, Aug 5, 2008 at 5:08 AM, Marco Gerards <address@hidden> wrote:
> Actually, I would prefer:
>
> pxe --info
>
> pxe --blksize=size
>
> pxe --unload
>
> You kinda reimplemented an argument parser.  The advantage of the
> build in argument parser is that it supports generation of --help
> documentation and it supports tab completion.

Ok.

>>
>>          /* Root drive will default to boot drive */
>> -        movb    $0xFF, %dh
>> +        movb $0xFF, %dh
>> +        movb $0x7F, %dl
>
> Please update the copyright year of files you change.  IIRC this file
> is not from this year.  I often forget to mention this, but it applies
> in general.
>
>

Oh, I forget that.

>> +  c.buffer = SEGOFS (GRUB_MEMORY_MACHINE_SCRATCH_ADDR);
>> +  while (pn >= data->packet_number)
>> +    {
>> +      c.buffer_size = grub_pxe_blksize;
>> +      grub_pxe_call (GRUB_PXENV_TFTP_READ, &c);
>> +      if (c.status)
>> +        {
>> +          grub_error (GRUB_ERR_BAD_FS, "read fails");
>> +          return -1;
>> +        }
>> +      data->packet_number++;
>> +    }
>> +
>> +  grub_memcpy (buf, (char *) GRUB_MEMORY_MACHINE_SCRATCH_ADDR, len);
>
> Is it save to use this memory?
>

This is only used by the real mode service. Once the data is
retrieved, it's copied to another buffer.

>> +GRUB_MOD_INIT(pxe)
>> +{
>> +  (void) mod;                        /* To stop warning. */
>> +
>> +  grub_pxe_detect ();
>> +  if (grub_pxe_pxenv)
>> +    {
>> +      grub_disk_dev_register (&grub_pxe_dev);
>> +      grub_fs_register (&grub_pxefs_fs);
>
> filesystems belong in fs/

Perhaps I should place it in fs/i386/pc ?

>> +struct grub_pxenv
>> +{
>> +  grub_uint8_t signature[6]; /* 'PXENV+' */
>> +  grub_uint16_t version;     /* MSB = major, LSB = minor */
>> +  grub_uint8_t length;               /* structure length */
>> +  grub_uint8_t checksum;     /* checksum pad */
>> +  grub_uint32_t rm_entry;    /* SEG:OFF to PXE entry point */
>> +  grub_uint32_t      pm_offset;      /* Protected mode entry */
>> +  grub_uint16_t pm_selector; /* Protected mode selector */
>> +  grub_uint16_t stack_seg;   /* Stack segment address */
>> +  grub_uint16_t      stack_size;     /* Stack segment size (bytes) */
>> +  grub_uint16_t bc_code_seg; /* BC Code segment address */
>> +  grub_uint16_t      bc_code_size;   /* BC Code segment size (bytes) */
>> +  grub_uint16_t      bc_data_seg;    /* BC Data segment address */
>> +  grub_uint16_t      bc_data_size;   /* BC Data segment size (bytes) */
>> +  grub_uint16_t      undi_data_seg;  /* UNDI Data segment address */
>> +  grub_uint16_t      undi_data_size; /* UNDI Data segment size (bytes) */
>> +  grub_uint16_t      undi_code_seg;  /* UNDI Code segment address */
>> +  grub_uint16_t      undi_code_size; /* UNDI Code segment size (bytes) */
>> +  grub_uint32_t pxe_ptr;     /* SEG:OFF to !PXE struct */
>> +} __attribute__ ((packed));
>
> Can you GRUB-ify the comments here and below a bit?

What do you mean by "GRUB-ify the comments" ?

-- 
Bean