Re: [Qemu-devel] [PATCH v3 1/6] Use IO port for qemu<->guest BIOS communication.

[Gleb Natapov <gleb@qumranet.com>]

On Sun, Aug 31, 2008 at 03:45:36PM +0300, Blue Swirl wrote:
> >  > I made an updated version of the patch 1/6, with explicit little
> >  > endian conversions. I'm not very happy with that. Another way would be
> >  > to add functions just to put different size numbers into device and
> >  > they would hide the conversion.
> >  >
> >
> > So what approach we should go with? We can go with the second one (add
> >  functions for each type) and extend interface as needed.
> 
> This version adds the functions, now the interface is much better. I
> updated the other patches too.

> +static int fw_cfg_select(FWCfgState *s, uint16_t key)
> +{
> +    int ret;
> +
> +    s->cur_offset = 0;
> +    if ((key & ~FW_CFG_ARCH_LOCAL) >= FW_CFG_MAX_ENTRY) {
> +        s->cur_entry = 0;
Here we select valid entry on incorrect input. Perhaps set 0xffff here
and return 0 on read if cur_entry == 0xffff ?

> +static uint8_t fw_cfg_read(FWCfgState *s)
> +{
> +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    FWCfgEntry *e = &s->entries[arch][s->cur_entry];
This should be:
	FWCfgEntry *e = &s->entries[arch][s->cur_entry & ~FW_CFG_ARCH_LOCAL];

> +    uint8_t ret;
> +
> +    if (!e || !e->data || s->cur_offset >= e->len)
Like this:
       if (s->cur_entry == 0xffff || !e->data || s->cur_offset >= e->len)

> +        ret = 0;
> +    else
> +        ret = e->data[s->cur_offset++];
> +
> +    FW_CFG_DPRINTF("read %d\n", ret);
> +
> +    return ret;
> +}
> +

Other then that looks great to me.

--
			Gleb.