Re: [PATCH v3] sim: cfi: new flash device simulation

[Mike Frysinger <vapier@gentoo.org>]

On Fri, Mar 25, 2011 at 11:15 AM, Frank Ch. Eigler wrote:
> vapier wrote:
>> +/* Clean up any state when this device is removed (e.g. when shutting down,
>> +   or when reloading via gdb).  */
>> +static void
>> +cfi_delete_callback (struct hw *me)
>> +{
>> +#ifdef HAVE_MMAP
>> +  struct cfi *cfi = hw_data (me);
>> +
>> +  if (cfi->mmap)
>> +    munmap (cfi->mmap, cfi->dev_size);
>> +#endif
>> +}
>
> Woudln't you want to write(2) out the contents of the flash backing
> store file, if !HAVE_MMAP?

perhaps, but that'd require quite a bit more logic that just a single
call to write() here.  frankly, i dont care about the systems that
dont support mmap as required by POSIX.  the ifdef's are only to keep
from breaking their builds.

if someone who actually cares about those systems wants to implement
that, then by all means.

>> +static void
>> +attach_cfi_regs (struct hw *me, struct cfi *cfi)
>> +[...]
>> +      cfi->data = HW_NALLOC (me, unsigned char, cfi->dev_size);
>> +      if (fd != -1)
>> +     read_len = read (fd, cfi->data, cfi->dev_size);
>
> It's more typical to loop in read(2), in case of an EINTR or somesuch
> temporary-failure return code.  Or use fread(3)?

i didnt bother with the EINTR case as i thought it overkill.  i did at
least make sure that the code would (for the most part) continue to
work in the case of a short read.

i wasnt aware that fread() would handle this for me though.  i dont
mind using that since it'd still be one fread() call.
-mike