On 28/06/2011 10:34, Stefan Hajnoczi wrote: > On Mon, Jun 27, 2011 at 5:12 PM, Fabien Chouteau <chout...@adacore.com> wrote: >> On 27/06/2011 17:39, Stefan Hajnoczi wrote: >>> On Mon, Jun 27, 2011 at 3:23 PM, Fabien Chouteau <chout...@adacore.com> >>> wrote: >>>> On 27/06/2011 15:50, Stefan Hajnoczi wrote: >>>>> On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chout...@adacore.com> >>>>> wrote: >>>>>> >>>>>> Signed-off-by: Fabien Chouteau <chout...@adacore.com> >>>>>> --- >>>>>> slirp/slirp.c | 4 ++-- >>>>>> 1 files changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> Any particular bug that this fixes? >>>>> >>>>> There have been 64 byte minimum padding patches to several emulated >>>>> NICs. There has also been discussion about where the best place to do >>>>> this is. Why is this patch necessary? >>>>> >>>> >>>> This patch is necessary because some NICs are configured to drop short >>>> frames, >>>> therefore the OS will not receive some of the packets generated by Qemu. >>>> >>>> There's a first patch to fix this issue: >>>> http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813 >>>> >>>> My patch fixes two other sources of short frames. >>> >>> Thanks for the explanation. I stepped back from the discussion on >>> where the right place to fix this is last time around. Now I'm >>> wondering why do anything in slirp when the other sources (tap, ...) >>> aren't padding to 64 bytes? >> >> I think that packets generated by Qemu must follow RFC. For other sources, >> qemu >> should keep original size when possible and put padding otherwise. > > slirp interfaces to net.h which does not emulate Ethernet. For > example, it doesn't carry the Frame Check Sequence (FCS) as per the > Ethernet standard. NICs that want to report FCS to the guest > calculate it themselves, just like they do with 64-byte padding. So I > find it weird to say we should honor Ethernet when the transport we > are using is not Ethernet.
In this case it's always Ethernet frames, look at the two patched lines, there's an Ethernet header: uint8_t arp_req[max(ETH_HLEN + sizeof(struct arphdr), 64)]; slirp_output(slirp->opaque, buf, max(ip_data_len + ETH_HLEN, 64)); > > This patch doesn't hurt but we'd be just as well off without it. > > Did you do this to fix a bug? If so, then something else in QEMU > needs to be fixed, not slirp. When Qemu generates bad Ethernet frames, I think it's a bug. And again, this is the extension of a previous patch. If this patch is not valid then we should revert the first, it's also a question of consistency. -- Fabien Chouteau
