On Fri 31 Jul 2009 03:46, Alessandro Rubini pondered:
> Thanks for your comments.
>
> >> +#ifndef CONFIG_TFTP_MAXBLOCK
> >> +#define CONFIG_TFTP_MAXBLOCK 16384
> >
> > It is more than tftp - nfs could also use the same.
>
> Yes, I know. But most users are tftp ones. And if you want an even
> number (like 16k) as a tftp packet you need to add the headers and the
> sequence count. And I prefer to have the useful number in the config.
> So I used "TFTP" in the name in order for NFS users to know they must
> make some calculation.
>
> > How about CONFIG_NET_MAXDEFRAG instead?
>
> We could have MAXPAYLOAD if we count in NFS overhead as well (I don't
> know how much it is, currently. Hope you see my point.
Not really.
IMHO - The protocol max payload should be taken care of on the protocol side,
not the common network side.
It then becomes:
#define TFTP_MTU_BLOCKSIZE (CONFIG_NET_MAXDEFRAG - TFTP_OVERHEAD)
#define NFS_READ_SIZE (CONFIG_NET_MAXDEFRAG - NFS_OVERHEAD)
or something like that (since NFS likes to be power of two, 1024, 2048, etc -
it would need to be tweaked a little), but you get the idea...
> >> +static IP_t *__NetDefragment(IP_t *ip, int *lenp)
> >> +{
> >
> > I don't understand the purpose of the lenp.
> >
> > The calling function doesn't use the len var, except for
> > ICMP_ECHO_REQUEST, which are not allowed to be fragmented.
> >
> > I eliminated it - and suffered no side effects.
>
> Well, since the caller has this "len" variable, I didn't want to leave
> it corrupted. But if it's actually unused after this point, we can well
> discard it.
OK.
> >> +#else /* !CONFIG_IP_DEFRAG */
> >> +
> >> +static inline IP_t *NetDefragment(IP_t *ip, int *lenp)
> >> +{
> >> + return ip;
> >> +}
> >> +#endif
> >
> > This needs to have the same logic (ip_off & (IP_OFFS | IP_FLAGS_MFRAG)) as
> > the above function. See comment below.
>
> Yes, correct. Thanks.
Were you going to send an update for Ben?
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
