[PATCH 0/3] batch: don't require checking retvalue of some bitmap ops

Nikolay Shirokovskiy posted 3 patches 2 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1596119258-782762-1-git-send-email-nshirokovskiy@virtuozzo.com
src/conf/capabilities.c           |  2 +-
src/conf/checkpoint_conf.c        |  2 +-
src/conf/domain_addr.c            |  8 ++++----
src/conf/domain_conf.c            |  9 ++++-----
src/conf/node_device_conf.c       |  2 +-
src/conf/snapshot_conf.c          |  2 +-
src/conf/storage_conf.c           |  2 +-
src/libxl/libxl_capabilities.c    |  2 +-
src/network/bridge_driver.c       |  6 +++---
src/qemu/qemu_capabilities.c      |  4 ++--
src/qemu/qemu_domain.c            |  2 +-
src/qemu/qemu_domain_address.c    |  4 ++--
src/qemu/qemu_driver.c            |  8 ++++----
src/qemu/qemu_hotplug.c           |  6 +++---
src/qemu/qemu_migration_cookie.c  |  8 ++++----
src/qemu/qemu_migration_params.c  | 28 +++++++++++++---------------
src/qemu/qemu_monitor.c           |  2 +-
src/qemu/qemu_slirp.c             |  2 +-
src/test/test_driver.c            |  2 +-
src/util/virbitmap.h              |  6 +++---
src/util/vircommand.c             |  2 +-
src/util/virdnsmasq.c             |  2 +-
src/util/virhostcpu.c             |  2 +-
src/util/virjson.c                |  2 +-
src/util/virnetdev.c              | 10 +++++-----
src/util/virnuma.c                |  4 ++--
src/util/virprocess.c             |  4 ++--
src/util/virresctrl.c             |  2 +-
src/util/virstoragefile.c         |  2 +-
src/vmx/vmx.c                     |  2 +-
tests/qemumonitorjsontest.c       |  2 +-
tests/testutils.c                 |  2 +-
tests/virbitmaptest.c             | 28 ++++++++++++++--------------
tools/virsh-domain.c              |  4 ++--
tools/virt-host-validate-common.c |  2 +-
35 files changed, 87 insertions(+), 90 deletions(-)

[PATCH 0/3] batch: don't require checking retvalue of some bitmap ops

Posted by Nikolay Shirokovskiy 2 weeks ago
Most of bitmap setBit/clearBit/getBit users know that the bitmap index is
not out of bound and thus don't check the return value. More precisely
the stats is next:

Method                all   check
===================================
virBitmapSetBit        85      14
virBitmapClearBit      15       3
virBitmapGetBit        15       6

where 'all' is the number of all occurences of the method and 'check' is the
number of occurences with 'if (method' pattern.

Thus keeping the retvalue checking requirement produces more
noise then helps. I guess we even can make these function return
void as users can simply compare the index with the bitmap size.

The removing of ignore_value was done by sed together with several manual
editings where methods calls were splitted across two lines.

FILES=`git grep -l 'ignore_value(virBitmapGetBit('`
sed -ibak -re 's/ignore_value\(virBitmapGetBit\((.*)\)\);/virBitmapGetBit(\1\);/' "$FILES"

Nikolay Shirokovskiy (3):
  batch: don't require checking retvalue for virBitmapSetBit
  batch: don't require checking retvalue for virBitmapClearBit
  batch: don't require checking retvalue for virBitmapGetBit

 src/conf/capabilities.c           |  2 +-
 src/conf/checkpoint_conf.c        |  2 +-
 src/conf/domain_addr.c            |  8 ++++----
 src/conf/domain_conf.c            |  9 ++++-----
 src/conf/node_device_conf.c       |  2 +-
 src/conf/snapshot_conf.c          |  2 +-
 src/conf/storage_conf.c           |  2 +-
 src/libxl/libxl_capabilities.c    |  2 +-
 src/network/bridge_driver.c       |  6 +++---
 src/qemu/qemu_capabilities.c      |  4 ++--
 src/qemu/qemu_domain.c            |  2 +-
 src/qemu/qemu_domain_address.c    |  4 ++--
 src/qemu/qemu_driver.c            |  8 ++++----
 src/qemu/qemu_hotplug.c           |  6 +++---
 src/qemu/qemu_migration_cookie.c  |  8 ++++----
 src/qemu/qemu_migration_params.c  | 28 +++++++++++++---------------
 src/qemu/qemu_monitor.c           |  2 +-
 src/qemu/qemu_slirp.c             |  2 +-
 src/test/test_driver.c            |  2 +-
 src/util/virbitmap.h              |  6 +++---
 src/util/vircommand.c             |  2 +-
 src/util/virdnsmasq.c             |  2 +-
 src/util/virhostcpu.c             |  2 +-
 src/util/virjson.c                |  2 +-
 src/util/virnetdev.c              | 10 +++++-----
 src/util/virnuma.c                |  4 ++--
 src/util/virprocess.c             |  4 ++--
 src/util/virresctrl.c             |  2 +-
 src/util/virstoragefile.c         |  2 +-
 src/vmx/vmx.c                     |  2 +-
 tests/qemumonitorjsontest.c       |  2 +-
 tests/testutils.c                 |  2 +-
 tests/virbitmaptest.c             | 28 ++++++++++++++--------------
 tools/virsh-domain.c              |  4 ++--
 tools/virt-host-validate-common.c |  2 +-
 35 files changed, 87 insertions(+), 90 deletions(-)

-- 
1.8.3.1

Re: [PATCH 0/3] batch: don't require checking retvalue of some bitmap ops

Posted by Peter Krempa 2 weeks ago
On Thu, Jul 30, 2020 at 17:27:35 +0300, Nikolay Shirokovskiy wrote:
> Most of bitmap setBit/clearBit/getBit users know that the bitmap index is
> not out of bound and thus don't check the return value. More precisely
> the stats is next:
> 
> Method                all   check
> ===================================
> virBitmapSetBit        85      14
> virBitmapClearBit      15       3
> virBitmapGetBit        15       6
> 
> where 'all' is the number of all occurences of the method and 'check' is the
> number of occurences with 'if (method' pattern.
> 
> Thus keeping the retvalue checking requirement produces more
> noise then helps. I guess we even can make these function return
> void as users can simply compare the index with the bitmap size.

Well. An ignore_value is not really expensive and it makes the callers
aware that something needs to be checked.

I don't really see the point of this.

Additionally, individual patches are missing justification in the commit
message. Mentioning it in the cover letter is not enough as that doesn't
get comitted.

Re: [PATCH 0/3] batch: don't require checking retvalue of some bitmap ops

Posted by Nikolay Shirokovskiy 2 weeks ago

On 30.07.2020 17:56, Peter Krempa wrote:
> On Thu, Jul 30, 2020 at 17:27:35 +0300, Nikolay Shirokovskiy wrote:
>> Most of bitmap setBit/clearBit/getBit users know that the bitmap index is
>> not out of bound and thus don't check the return value. More precisely
>> the stats is next:
>>
>> Method                all   check
>> ===================================
>> virBitmapSetBit        85      14
>> virBitmapClearBit      15       3
>> virBitmapGetBit        15       6
>>
>> where 'all' is the number of all occurences of the method and 'check' is the
>> number of occurences with 'if (method' pattern.
>>
>> Thus keeping the retvalue checking requirement produces more
>> noise then helps. I guess we even can make these function return
>> void as users can simply compare the index with the bitmap size.
> 
> Well. An ignore_value is not really expensive and it makes the callers
> aware that something needs to be checked.

The only condition these methods can fail is out of bound. Most of
time it is known by the caller that there is no out of bound condition.
So when compiler suggests to check error I personally go and read
the code only to found it can not fail in my circumstances.
At the same time it is quite obvious that these function can not
produce something meaningful for out of bound. That's why
I even thinking of why don't make these methods return void. 

> 
> I don't really see the point of this.
> 
> Additionally, individual patches are missing justification in the commit
> message. Mentioning it in the cover letter is not enough as that doesn't
> get comitted.
> 

I thought that writing same justification 3 times in a row will be
too much. At the same time writing some short version will not explain
things thoroughly. May be I should add good explanation only
to the first patch.

Nikolay

Re: [PATCH 0/3] batch: don't require checking retvalue of some bitmap ops

Posted by Nikolay Shirokovskiy 1 day ago

On 30.07.2020 18:49, Nikolay Shirokovskiy wrote:
> 
> 
> On 30.07.2020 17:56, Peter Krempa wrote:
>> On Thu, Jul 30, 2020 at 17:27:35 +0300, Nikolay Shirokovskiy wrote:
>>> Most of bitmap setBit/clearBit/getBit users know that the bitmap index is
>>> not out of bound and thus don't check the return value. More precisely
>>> the stats is next:
>>>
>>> Method                all   check
>>> ===================================
>>> virBitmapSetBit        85      14
>>> virBitmapClearBit      15       3
>>> virBitmapGetBit        15       6
>>>
>>> where 'all' is the number of all occurences of the method and 'check' is the
>>> number of occurences with 'if (method' pattern.
>>>
>>> Thus keeping the retvalue checking requirement produces more
>>> noise then helps. I guess we even can make these function return
>>> void as users can simply compare the index with the bitmap size.
>>
>> Well. An ignore_value is not really expensive and it makes the callers
>> aware that something needs to be checked.
> 
> The only condition these methods can fail is out of bound. Most of
> time it is known by the caller that there is no out of bound condition.
> So when compiler suggests to check error I personally go and read
> the code only to found it can not fail in my circumstances.
> At the same time it is quite obvious that these function can not
> produce something meaningful for out of bound. That's why
> I even thinking of why don't make these methods return void. 
> 
>>
>> I don't really see the point of this.
>>
>> Additionally, individual patches are missing justification in the commit
>> message. Mentioning it in the cover letter is not enough as that doesn't
>> get comitted.
>>
> 
> I thought that writing same justification 3 times in a row will be
> too much. At the same time writing some short version will not explain
> things thoroughly. May be I should add good explanation only
> to the first patch.
> 

Hi, everyone.

Is there other opinions on the topic or I can forget about the series and
let it go?)

Nikolay