Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/2/2018 4:48 PM, Maxime Coquelin wrote:
> 
> 
> On 11/2/18 4:19 PM, Chas Williams wrote:
>>
>>
>> On 11/02/2018 10:33 AM, Ferruh Yigit wrote:
>>> On 11/1/2018 2:45 PM, Luca Boccassi wrote:
>>>> On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
>>>>> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
>>>>> those routines doesn't need repeated.  Use started and opened to
>>>>> track
>>>>> the adapter's status.
>>>>>
>>>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>>
>>> <...>
>>>
>>>>> @@ -253,7 +254,7 @@ struct virtio_hw {
>>>>>       uint64_t    req_guest_features;
>>>>>       uint64_t    guest_features;
>>>>>       uint32_t    max_queue_pairs;
>>>>> -    uint16_t    started;
>>>>> +    bool        started;
>>>>>       uint16_t    max_mtu;
>>>>>       uint16_t    vtnet_hdr_size;
>>>>>       uint8_t        vlan_strip;
>>>>> @@ -268,6 +269,7 @@ struct virtio_hw {
>>>>>       struct virtio_pci_common_cfg *common_cfg;
>>>>>       struct virtio_net_config *dev_cfg;
>>>>>       void        *virtio_user_dev;
>>>>> +    bool        opened;
>>>
>>> This is already merged into next-net-virtio, but I would like to 
>>> hightlight the
>>> checkpatch warning about `bool` usage in struct [1].
>>> Briefly it suggests preferring primitive data types against `bool` in 
>>> structures
>>> because its size is not clear.
>>>
>>> What do you think about it, do you have strong opinion to have them as 
>>> bool?
>>
>> Yes, I suppose I do.  bool is the "proper" representation for yes/no.
>> The arguments cited aren't convincing.
>>
>> The size of bool is the size of bool. The compiler gets to make that
>> decision.  I don't get to decide the size of int either.  The size and
>> alignemnt of bool should be optimal because your compiler probably knows
>> more about the target than you do.  If your compiler can't figure out
>> alignment in a structure, please fix the compiler.
>>
>> bool is a primitive data type per the C99 standard.
> 
> I would like to keep it as bool too.

OK

> 
>>> [1]
>>> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
>>> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
>>> #85: FILE: drivers/net/virtio/virtio_pci.h:234:
>>> +       bool        started;
>>>
>>> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
>>> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
>>> #93: FILE: drivers/net/virtio/virtio_pci.h:260:
>>> +       bool        opened;
>>>
> 
> BTW, I don't get this warning when running checkpatch, what kenrel
> version is it coming from?

v4.18-11078-gd729593e492e
d729593e492e ("checkpatch: add a --strict test for structs with bool member
definitions")

> 
> Thanks,
> Maxime
>