This is the mail archive of the ecos-bugs@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug 1001177] Redboot DHCP client race condition, XID, and retryproblems.


Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001177

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO
                 CC|                            |jifl@ecoscentric.com

--- Comment #2 from Jonathan Larmour <jifl@ecoscentric.com> 2011-03-24 17:36:49 GMT ---
Some comments: 

- in changelog "fixrace" -> "fix race". Also reference this issue number.

- It would be nice (although admittedly arguably an enhancement) for all of
bootp/dhcp support to be removable by config option. There are many instances
where redboot is only ever given a static IP, so this code is just taking up
space.

If that happens, obviously a few existing CDL config options would then live
under that CDL component.

I notice (in existing code, not your patch) that
CYGSEM_REDBOOT_NETWORKING_USE_GATEWAY depends on
CYGSEM_REDBOOT_NETWORKING_DHCP, which seems spurious to me.

- In various places in the patch, constants like 4 or 6 are used. While
entirely accurate, and they'll never change, from the principle of
self-documenting code it would be nicer if they were things like e.g.
sizeof(b->bp_chaddr) or sizeof(__local_enet_addr) etc. I admit I'm being a bit
idealistic.

- It's not clear to me in bootp_handler that this supports BOOTP even when
CYGSEM_REDBOOT_NETWORKING_DHCP is enabled. Can you confirm it does? Just
because CYGSEM_REDBOOT_NETWORKING_DHCP is enabled, shouldn't mean that we can't
work with BOOTP any more. This may have been a problem in the previous code
too.

- You now depend on the CRC code for xid generation. That's quite a chunky
dependency to introduce, and slightly pointless given there's very little
randomness from every byte of the MAC address. I think it would be good enough
to just take the lower 4 bytes of the MAC (or if you prefer, take the bottom
four, and then XOR in the top 2). You could maybe XOR in MS_TICKS() for a
little more entropy, although not much I agree. Or for a bit more, XOR in &xid
and &__bootp_find_local_ip thus making it more specific to the specific build
of the executable.

I know that CYGPKG_CRC comes from the template, but that doesn't mean the code
is included - only if it's used. Admittedly we should also add on explicit CDL
dependencies where it's already been used - it's rather lax that that hasn't
been done for CYGBLD_BUILD_REDBOOT_WITH_CKSUM and CYGSEM_REDBOOT_FIS_CRC_CHECK.

Formally, I'll mark this patch as not having passed review, even though the
changes are minor. Also please don't set the assignment flag until we hear from
the FSF. I still haven't had a reply from them. I have no doubt you filled in
the assignment, but we've had things before when a company has made changes to
the assignment text, or changes to the copyright disclaimer, or has omitted the
disclaimer entirely in what they sent. So I want to allow for the possibility
that something goes wrong. The copyright is only actually assigned once the FSF
executes the assignment on their side too, so until then, the copyright still
rests with you, even if you've signed.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]