Re: [PATCH v2 1/3] MdeModulePkg/Dhcp4Dxe: Remove unnecessary NULL pointer check.

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi,

On 1/21/19 1:53 PM, Gao, Liming wrote:
> Thanks Ard and Laszlo. For the minor change in single patch, the patch may be sent separately with the clear subject. Or, the patch set can be sent again.

Since it is hard to follow technical discussion when top-posted (see
https://www.caliburn.nl/topposting.html) without scrolling and sometime
loosing context, can we gently suggest bottom-posting in edk2-devel
etiquette? (No offence, this is a humble suggestion from a not very
active reviewer to a highly active contributor, but this might ease the
on-list review workflow).

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Saturday, January 19, 2019 9:17 AM
>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Ye,
>> Ting <ting.ye@intel.com>
>> Subject: Re: [PATCH v2 1/3] MdeModulePkg/Dhcp4Dxe: Remove unnecessary NULL pointer check.
>>
>> On 01/18/19 12:09, Ard Biesheuvel wrote:
>>> On Fri, 18 Jan 2019 at 06:38, Gao, Liming <liming.gao@intel.com> wrote:
>>>>
>>>> This is my idea to avoid the duplicated mail. I also include Ard and Laszlo to collect the feedback on how to handle the partial update in
>> the patchset.
>>>>
>>>
>>> Laszlo may disagree with me, but I think that it is not always
>>> necessary to resend the entire series when only a single patch
>>> changes. It does depend on the situation, though: if it is a trivial
>>> patch in a more complicated series then it might make little sense. In
>>> other case, just resending the whole thing is probably better.
>>
>> I think resending one patch can be acceptable, but the subject line
>> (patch nr) and the threading have to be correct. Also, I don't think
>> this approach scales beyond exactly one patch-update; it's easy to lose
>> track of version numbers etc. So "use sparingly" I guess? :)

For a 3 patches series, I wouldn't worry resending the whole series...

The 'git backport-diff' tool is very powerful to resume differencies
between 2 series, in particular when the project evolved between
versions of a series (simplest example: a rebase):

https://github.com/codyprime/git-scripts/blob/master/git-backport-diff#L27

Sadly I can't find a distribution handy package that provides it, so it
has to be installed manually.

Regards,

Phil.

>>>>> -----Original Message-----
>>>>> From: Wu, Jiaxin
>>>>> Sent: Friday, January 18, 2019 1:32 PM
>>>>> To: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
>>>>> Cc: Ye, Ting <ting.ye@intel.com>; Gao, Liming <liming.gao@intel.com>
>>>>> Subject: RE: [PATCH v2 1/3] MdeModulePkg/Dhcp4Dxe: Remove unnecessary NULL pointer check.
>>>>>
>>>>> Just confirmed with Liming, we don't need to seed the full series patches if only one is updated.
>>>>>
>>>>> Thanks,
>>>>> jiaxin
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Fu, Siyuan
>>>>>> Sent: Friday, January 18, 2019 1:29 PM
>>>>>> To: Wu, Hao A <hao.a.wu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>;
>>>>>> edk2-devel@lists.01.org
>>>>>> Cc: Ye, Ting <ting.ye@intel.com>; Gao, Liming <liming.gao@intel.com>
>>>>>> Subject: RE: [PATCH v2 1/3] MdeModulePkg/Dhcp4Dxe: Remove
>>>>>> unnecessary NULL pointer check.
>>>>>>
>>>>>> Hi, Jiaxin
>>>>>>
>>>>>> Yes the full patch series is needed for a v2 version.
>>>>>>
>>>>>> And also, why you removed the "(Instance->Token != NULL)" check in the if
>>>>>> condition?
>>>>>>
>>>>>> BestRegards
>>>>>> Fu Siyuan
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Wu, Hao A
>>>>>>> Sent: Friday, January 18, 2019 1:22 PM
>>>>>>> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
>>>>>>> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Gao,
>>>>>>> Liming <liming.gao@intel.com>
>>>>>>> Subject: RE: [PATCH v2 1/3] MdeModulePkg/Dhcp4Dxe: Remove
>>>>>> unnecessary NULL
>>>>>>> pointer check.
>>>>>>>
>>>>>>> Hi Jiaxin,
>>>>>>>
>>>>>>> A comment that is not related with the content of the patch itself:
>>>>>>> Please help to send the full patch series when a new version is needed.
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Hao Wu
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Wu, Jiaxin
>>>>>>>> Sent: Friday, January 18, 2019 1:16 PM
>>>>>>>> To: edk2-devel@lists.01.org
>>>>>>>> Cc: Ye, Ting; Fu, Siyuan; Wu, Hao A; Gao, Liming; Wu, Jiaxin
>>>>>>>> Subject: [PATCH v2 1/3] MdeModulePkg/Dhcp4Dxe: Remove
>>>>>> unnecessary
>>>>>>>> NULL pointer check.
>>>>>>>>
>>>>>>>> v2: The DHCP Instance might be destroyed in PxeDhcpDone. So,
>>>>>>>> we need safe-delete.
>>>>>>>>
>>>>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1469
>>>>>>>>
>>>>>>>> Since the value of Instance is retrieved from the list Entry,
>>>>>>>> it can't be the NULL pointer, so just remove the unnecessary
>>>>>>>> check.
>>>>>>>>
>>>>>>>> Cc: Ye Ting <ting.ye@intel.com>
>>>>>>>> Cc: Fu Siyuan <siyuan.fu@intel.com>
>>>>>>>> Cc: Wu Hao A <hao.a.wu@intel.com>
>>>>>>>> Cc: Gao Liming <liming.gao@intel.com>
>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>>>> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
>>>>>>>> ---
>>>>>>>>  MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 11 ++++-----
>>>>>> --
>>>>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
>>>>>>>> b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
>>>>>>>> index 98a22a77b4..780f8b4224 100644
>>>>>>>> --- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
>>>>>>>> +++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
>>>>>>>> @@ -1,9 +1,9 @@
>>>>>>>>  /** @file
>>>>>>>>    EFI DHCP protocol implementation.
>>>>>>>>
>>>>>>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>>>>>>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>>>>>>>>  This program and the accompanying materials
>>>>>>>>  are licensed and made available under the terms and conditions of the
>>>>>> BSD
>>>>>>>> License
>>>>>>>>  which accompanies this distribution.  The full text of the license may be
>>>>>>>> found at
>>>>>>>>  http://opensource.org/licenses/bsd-license.php
>>>>>>>>
>>>>>>>> @@ -1646,16 +1646,13 @@ ON_EXIT:
>>>>>>>>    //
>>>>>>>>    // Iterate through all the DhcpSb Children.
>>>>>>>>    //
>>>>>>>>    NET_LIST_FOR_EACH_SAFE (Entry, Next, &DhcpSb->Children) {
>>>>>>>>      Instance = NET_LIST_USER_STRUCT (Entry, DHCP_PROTOCOL, Link);
>>>>>>>> -
>>>>>>>> -    if ((Instance != NULL) && (Instance->Token != NULL)) {
>>>>>>>> -      Instance->Timeout--;
>>>>>>>> -      if (Instance->Timeout == 0) {
>>>>>>>> -        PxeDhcpDone (Instance);
>>>>>>>> -      }
>>>>>>>> +    Instance->Timeout--;
>>>>>>>> +    if (Instance->Timeout == 0) {
>>>>>>>> +      PxeDhcpDone (Instance);
>>>>>>>>      }
>>>>>>>>    }
>>>>>>>>
>>>>>>>>    return ;
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.17.1.windows.2