From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 479E71B107 for ; Fri, 2 Nov 2018 17:48:10 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9D361307D846; Fri, 2 Nov 2018 16:48:09 +0000 (UTC) Received: from [10.36.112.50] (ovpn-112-50.ams2.redhat.com [10.36.112.50]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A15EF5FC2C; Fri, 2 Nov 2018 16:48:07 +0000 (UTC) To: Chas Williams <3chas3@gmail.com>, Ferruh Yigit , Luca Boccassi , dev@dpdk.org References: <1500332723-10812-1-git-send-email-ciwillia@brocade.com> <1500332723-10812-2-git-send-email-ciwillia@brocade.com> <1541083532.4849.8.camel@debian.org> From: Maxime Coquelin Message-ID: <2997986d-11f6-c170-087f-ff08c067d93c@redhat.com> Date: Fri, 2 Nov 2018 17:48:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Fri, 02 Nov 2018 16:48:09 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Nov 2018 16:48:10 -0000 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 >> >> <...> >> >>>> @@ -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. >> [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? Thanks, Maxime