I think the summary here is - nobody cares about ArmBds (it will be deleted asap anyway) - the patch is technically no worse than what's there already - the original code should have dynamically allocated the buffer, but doesn't - the code should have given a sensible error message mentioning that the file size wasn't returned by the server, but doesn't - Ashu is using the "wrong" tftp server, which is why he found this problem
So I'd say we just submit the patch. Until we get around to deleting ArmBds, it's one less chance for other innocent victims to stumble across this problem and come to us for help. On 6 November 2015 at 14:22, Laszlo Ersek <ler...@redhat.com> wrote: > On 11/06/15 15:19, Laszlo Ersek wrote: >> On 11/06/15 14:31, Ashutosh Singh wrote: >>>> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit >>>> of code duplicates functionality already available elsewhere in the >>>> tree ... but I also don't care much about ArmBds. >>>> >>>> What I'm curious about is - why is your Mtftp4GetFileSize call failing >>>> such that you need to use a hardcoded buffer size? >>>> Does your TFTP server not support rfc2349? >>>> Would changing that (for example by switching to tftpd-hpa over the >>>> unmaintained netkit-tftpd) not make more sense? >>> >>> Took a while to test with tftpd-hpa, with this it does work fine, i.e. >>> Mtftp4GetFileSize does succeed and buffer size is allocated accordingly. >>> I would still request you to merge the change, as it will mean people >>> using the default tftpd that comes with ubuntu distribution >> >> You mean the one: >> >> http://packages.ubuntu.com/wily/tftpd >> http://packages.ubuntu.com/xenial/tftpd >> >> that is explicitly advertised in Debian (Ubuntu's parent distro): >> >> https://packages.debian.org/jessie/tftpd >> >> and in Ubuntu as well: >> >> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz >> >> as unsuitable for PXE booting? >> >> See also >> >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612 >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638 >> >>> will also >>> be able to use the tftp boot for ARM platforms. >> >> Since this patch intends to modify the ARM BDS, I don't really have a >> horse in the race, but please be aware that this is just a band-aid. >> Debian and Ubuntu users should use the right tftp server (and/or file a >> bug with their distro to change the default TFTP server). >> >> I recommend to state the above in the commit message. > > (Sorry about the followup.) > > Also, the subject should say: > > ArmPkg: BdsLib: increase tftp buffer size > > (When I first saw the email, I thought the patch was for MdeModulePkg or > NetworkPkg.) > > Thanks > Laszlo > >> >>> >>> -----Original Message----- >>> From: Leif Lindholm [mailto:leif.lindh...@linaro.org] >>> Sent: 05 November 2015 12:26 >>> To: Ashutosh Singh >>> Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; >>> ryan.har...@linaro.org; James King >>> Subject: Re: [PATCH] Increase tftp buffer size >>> >>> On Thu, Nov 05, 2015 at 11:37:43AM +0000, Ashutosh Singh wrote: >>>> Default tftp buffer size (8MB) is too small for standard >>>> ARM kernel images, which results in multiple aborted tftp fetches. >>>> This fix increase the buffer size to 16MB. >>> >>> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit >>> of code duplicates functionality already available elsewhere in the >>> tree ... but I also don't care much about ArmBds. >>> >>> What I'm curious about is - why is your Mtftp4GetFileSize call failing >>> such that you need to use a hardcoded buffer size? >>> Does your TFTP server not support rfc2349? >>> Would changing that (for example by switching to tftpd-hpa over the >>> unmaintained netkit-tftpd) not make more sense? >>> >>> / >>> Leif >>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Ashutosh Singh <ashutosh.si...@arm.com> >>>> --- >>>> ArmPkg/Library/BdsLib/BdsFilePath.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c >>>> b/ArmPkg/Library/BdsLib/BdsFilePath.c >>>> index ff42175..0410236 100644 >>>> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c >>>> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c >>>> @@ -1198,7 +1198,7 @@ BdsTftpLoadImage ( >>>> if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, &FileSize) == >>>> EFI_SUCCESS) { >>>> TftpBufferSize = FileSize; >>>> } else { >>>> - TftpBufferSize = SIZE_8MB; >>>> + TftpBufferSize = SIZE_16MB; >>>> } >>>> >>>> TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT)); >>>> @@ -1209,7 +1209,7 @@ BdsTftpLoadImage ( >>>> TftpContext->FileSize = FileSize; >>>> >>>> for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize); >>>> - TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) { >>>> + TftpBufferSize = (TftpBufferSize + SIZE_16MB) & >>>> (~(SIZE_16MB-1))) { >>>> // >>>> // Allocate a buffer to hold the whole file. >>>> // >>>> -- >>>> 1.7.9.5 >>>> >>>> >>>> ________________________________ >>>> >>>> -- IMPORTANT NOTICE: The contents of this email and any attachments are >>>> confidential and may also be privileged. If you are not the intended >>>> recipient, please notify the sender immediately and do not disclose the >>>> contents to any other person, use it for any purpose, or store or copy the >>>> information in any medium. Thank you. >>>> >>> >>> >>> ________________________________ >>> >>> -- IMPORTANT NOTICE: The contents of this email and any attachments are >>> confidential and may also be privileged. If you are not the intended >>> recipient, please notify the sender immediately and do not disclose the >>> contents to any other person, use it for any purpose, or store or copy the >>> information in any medium. Thank you. >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
