Due to the fact that I spent lots of time on the shell which is an application that can be loaded on firmware anywhere and is frequently used during the testing and development of new firmware, I found almost the entire sting manipulation function set to be so ASSERT filled that it was very unhelpful. I like return values way more than I do asserts. The same way we have libraries in EDKII that have constructors and no destructors which means they are risky and potentially useless for any code that must support unload.
-Jaben > On May 18, 2016, at 9:57 AM, Carsey, Jaben <jaben.car...@intel.com> wrote: > > Sure. > > Reviewed by: Jaben Carsey <jaben.car...@intel.com> > > -Jaben > >> On May 18, 2016, at 9:44 AM, El-Haj-Mahmoud, Samer >> <samer.el-haj-mahm...@hpe.com> wrote: >> >> Thanks Jaben! >> >> Will you give it a reviewed-by ? >> >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Carsey, Jaben >> Sent: Wednesday, May 18, 2016 11:25 AM >> To: Palmer, Thomas <thomas.pal...@hpe.com>; jim_dai...@dell.com; >> edk2-devel@lists.01.org >> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Carsey, Jaben >> <jaben.car...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Gao, Liming >> <liming....@intel.com> >> Subject: Re: [edk2] [PATCH] [MdePkg/BaseLib]: Remove overreaction to >> SourceLen in StrCpyS >> >> >> >>> -----Original Message----- >>> From: Palmer, Thomas [mailto:thomas.pal...@hpe.com] >>> Sent: Wednesday, May 18, 2016 9:21 AM >>> To: jim_dai...@dell.com; Carsey, Jaben <jaben.car...@intel.com>; edk2- >>> de...@lists.01.org >>> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Yao, Jiewen >>> <jiewen....@intel.com>; Gao, Liming <liming....@intel.com> >>> Subject: RE: [edk2] [PATCH] [MdePkg/BaseLib]: Remove overreaction to >>> SourceLen in StrCpyS >>> Importance: High >>> >>> >>> My comment at bottom. Sorry, HPE is an Outlook shop. >>> >>> -----Original Message----- >>> From: jim_dai...@dell.com [mailto:jim_dai...@dell.com] >>> Sent: Wednesday, May 18, 2016 11:17 AM >>> To: jaben.car...@intel.com; Palmer, Thomas <thomas.pal...@hpe.com>; >>> edk2-devel@lists.01.org >>> Cc: michael.d.kin...@intel.com; jiewen....@intel.com; >>> liming....@intel.com >>> Subject: RE: [edk2] [PATCH] [MdePkg/BaseLib]: Remove overreaction to >>> SourceLen in StrCpyS >>> >>> Dell Customer Communication >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf >>>>> Of Thomas Palmer >>>>> Sent: Wednesday, May 18, 2016 9:01 AM >>>>> To: edk2-devel@lists.01.org >>>>> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Yao, Jiewen >>>>> <jiewen....@intel.com>; Gao, Liming <liming....@intel.com> >>>>> Subject: [edk2] [PATCH] [MdePkg/BaseLib]: Remove overreaction to >>>>> SourceLen in StrCpyS >>>>> >>>>> The StrCpyS function is "safe" because the destination buffer will >>>>> not be overwritten and the string will have a NULL terminator byte. >>>>> Today, StrCpyS will also refuse to copy the string if its length is >>>>> greater than the size of the destination buffer. However, this >>>>> behavior is not documented in the function's comment section. >>>>> >>>>> I will publicly speculate that most uses/users of StrCpyS expect it >>>>> to copy as much of the source string as possible instead of >>>>> ASSERT'ing or returning an error (and copying nothing) when the >>>> >>>> [Jaben] I disagree here. I think that returning the error and >>>> copying nothing >>> is expected. >>>> I agree that the ASSERT is really unpleasant though and I like removing it. >>> >>> [Jim] Thomas predicted my expectations *exactly*. And I must say I was >>> quite displeased when my use of a "safe" routine caused an ASSERT. >>> >>> [Thomas] Using my NIX "skills" on my slightly out-of-date tree: >>> fgrep StrCpyS . -R | fgrep ".c:" | fgrep -v "AsciiStrCpyS" | wc -l >>> >>> shows we have about 128 uses of StrCpyS. Of those ... >>> >>> fgrep StrCpyS . -R | fgrep ".c:" | fgrep -v "AsciiStrCpyS" | fgrep = >>> >>> I only see 1 that ever stored the return from StrCpyS. It seems then >>> that our >>> EDK2 developers expect StrCpyS to behave more like StrCpy in that it just >>> copies the string. If source length was important to our developers, they >>> either would have used StrnCpy/StrnCpyS or would be checking the >>> return value from StrCpyS. >>> >>> Respectfully, I'd like to get some more commentary on this. Could you >>> explain why a source length check is needed when StrnCpyS exists? That >>> would seem the most direct way to protect the code from using too much >>> source string. >> >> So I had to work around the whole thing due to the ASSERT. I (poorly >> phrased) that I understood the behavior correctly and was not confused about >> expectation. I was hoping to get the error when there was insufficient >> space, but alas, no. >> >> I support the change at this point. >> _______________________________________________ >> 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
