[PATCH 0/4] btrfs-progs: Compiling warning fixes for devel branch

[PATCH 0/4] btrfs-progs: Compiling warning fixes for devel branch

[Qu Wenruo]

We have several compiling errors, in devel branch.
One looks like a false alert from compiler, the first patch will
workaround it.

3 warning from libbtrfsutils are due to python3.8 changes.
Handle it properly by using designated initialization, which also saves
us quite some lines.

Qu Wenruo (4):
  btrfs-progs: check/lowmem: Fix a false alert on uninitialized value
  btrfs-progs: libbtrfsutil: Convert to designated initialization for
    BtrfsUtilError_type
  btrfs-progs: libbtrfsutil: Convert to designated initialization for
    QgroupInherit_type
  btrfs-progs: libbtrfsutil: Convert to designated initialization for
    SubvolumeIterator_type

 check/mode-common.c             |  2 +-
 libbtrfsutil/python/error.c     | 49 ++++++++-------------------------
 libbtrfsutil/python/qgroup.c    | 43 ++++++-----------------------
 libbtrfsutil/python/subvolume.c | 44 ++++++-----------------------
 4 files changed, 30 insertions(+), 108 deletions(-)

-- 
2.24.0

[PATCH 1/4] btrfs-progs: check/lowmem: Fix a false alert on uninitialized value

[Qu Wenruo]

[BUG]
When compiling the devel branch with commit fb8f05e40b458
("btrfs-progs: check: Make repair_imode_common() handle inodes in
subvolume trees"), the following warning will be reported:

  check/mode-common.c: In function ‘detect_imode’:
  check/mode-common.c|1071 col 23| warning: ‘imode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  1071 |   *imode_ret = (imode | 0700);
       |                ~~~~~~~^~~~~~~

This only occurs for regular build. If compiled with D=1, the warning
just disappears.

[CAUSE]
Looks like a bug in gcc optimization.
The code will only set @imode_ret when @found is true.
And for every "found = true" assignment we have assigned @imode.
So this is just a false alert.

[FIX]
I hope I can fix the problem of GCC, but obviously I can't (at least for
now).

So let's assign an initial value 0 to @imode to suppress the false
alert.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/mode-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check/mode-common.c b/check/mode-common.c
index 10ad6d228a03..9e81082b10aa 100644
--- a/check/mode-common.c
+++ b/check/mode-common.c
@@ -972,7 +972,7 @@ int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
 	struct btrfs_inode_item iitem;
 	bool found = false;
 	u64 ino;
-	u32 imode;
+	u32 imode = 0;
 	int ret = 0;
 
 	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
-- 
2.24.0

[PATCH 2/4] btrfs-progs: libbtrfsutil: Convert to designated initialization for BtrfsUtilError_type

[Qu Wenruo]

[BUG]
When compiling btrfs-progs with libbtrfsutil on a python3.8 system, we
got the following warning:

  error.c:169:2: warning: initialization of ‘long int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
    169 |  NULL,      /* tp_print */
        |  ^~~~
  error.c:169:2: note: (near initialization for ‘BtrfsUtilError_type.tp_vectorcall_offset’)

[CAUSE]
C definition of PyTypeObject changed in python 3.8.
Now at the old tp_print, we have tp_vectorcall_offset.

So we got above warning.

[FIX]
C has designated initialization, which can assign values to each named
member, without hard coding to match the offset.
Also, uninitialized values will be 0, so we can also save a lot of
unneeded "= 0" or "= NULL" lines.

Just use that awesome feature to avoid any future breakage.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 libbtrfsutil/python/error.c | 49 +++++++++----------------------------
 1 file changed, 12 insertions(+), 37 deletions(-)

diff --git a/libbtrfsutil/python/error.c b/libbtrfsutil/python/error.c
index 0876c9b42c81..b2076e6bf4d6 100644
--- a/libbtrfsutil/python/error.c
+++ b/libbtrfsutil/python/error.c
@@ -162,41 +162,16 @@ static PyMemberDef BtrfsUtilError_members[] = {
 
 PyTypeObject BtrfsUtilError_type = {
 	PyVarObject_HEAD_INIT(NULL, 0)
-	"btrfsutil.BtrfsUtilError",			/* tp_name */
-	sizeof(BtrfsUtilError),				/* tp_basicsize */
-	0,						/* tp_itemsize */
-	(destructor)BtrfsUtilError_dealloc,		/* tp_dealloc */
-	NULL,						/* tp_print */
-	NULL,						/* tp_getattr */
-	NULL,						/* tp_setattr */
-	NULL,						/* tp_as_async */
-	NULL,						/* tp_repr */
-	NULL,						/* tp_as_number */
-	NULL,						/* tp_as_sequence */
-	NULL,						/* tp_as_mapping */
-	NULL,						/* tp_hash  */
-	NULL,						/* tp_call */
-	(reprfunc)BtrfsUtilError_str,			/* tp_str */
-	NULL,						/* tp_getattro */
-	NULL,						/* tp_setattro */
-	NULL,						/* tp_as_buffer */
-	Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC,	/* tp_flags */
-	BtrfsUtilError_DOC,				/* tp_doc */
-	(traverseproc)BtrfsUtilError_traverse,		/* tp_traverse */
-	(inquiry)BtrfsUtilError_clear,			/* tp_clear */
-	NULL,						/* tp_richcompare */
-	0,						/* tp_weaklistoffset */
-	NULL,						/* tp_iter */
-	NULL,						/* tp_iternext */
-	NULL,						/* tp_methods */
-	BtrfsUtilError_members,				/* tp_members */
-	NULL,						/* tp_getset */
-	NULL,						/* tp_base */
-	NULL,						/* tp_dict */
-	NULL,						/* tp_descr_get */
-	NULL,						/* tp_descr_set */
-	offsetof(BtrfsUtilError, os_error.dict),	/* tp_dictoffset */
-	NULL,						/* tp_init */
-	NULL,						/* tp_alloc */
-	BtrfsUtilError_new,				/* tp_new */
+	.tp_name		= "btrfsutil.BtrfsUtilError",
+	.tp_basicsize		= sizeof(BtrfsUtilError),
+	.tp_dealloc		= (destructor)BtrfsUtilError_dealloc,
+	.tp_str			= (reprfunc)BtrfsUtilError_str,
+	.tp_flags		=  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE |
+				   Py_TPFLAGS_HAVE_GC,
+	.tp_doc			= BtrfsUtilError_DOC,
+	.tp_traverse		= (traverseproc)BtrfsUtilError_traverse,
+	.tp_clear		= (inquiry)BtrfsUtilError_clear,
+	.tp_members		= BtrfsUtilError_members,
+	.tp_dictoffset		= offsetof(BtrfsUtilError, os_error.dict),
+	.tp_new			= BtrfsUtilError_new,
 };
-- 
2.24.0

[PATCH 3/4] btrfs-progs: libbtrfsutil: Convert to designated initialization for QgroupInherit_type

[Qu Wenruo]

[BUG]
When compiling btrfs-progs with libbtrfsutil on a python3.8 system, we
got the following warning:

  qgroup.c:110:2: warning: initialization of ‘long int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
    110 |  NULL,     /* tp_print */
        |  ^~~~
  qgroup.c:110:2: note: (near initialization for ‘QgroupInherit_type.tp_vectorcall_offset’)

[CAUSE]
C definition of PyTypeObject changed in python 3.8.
Now at the old tp_print, we have tp_vectorcall_offset.

So we got above warning.

[FIX]
C has designated initialization, which can assign values to each named
member, without hard coding to match the offset.
And all the other uninitialized values will be set to 0, so we can save
a lot of unneeded "= 0" or "= NULL" lines.

Just use that awesome feature to avoid any future breakage.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 libbtrfsutil/python/qgroup.c | 43 +++++++-----------------------------
 1 file changed, 8 insertions(+), 35 deletions(-)

diff --git a/libbtrfsutil/python/qgroup.c b/libbtrfsutil/python/qgroup.c
index 44ac5ebc19d2..163fbbf077e6 100644
--- a/libbtrfsutil/python/qgroup.c
+++ b/libbtrfsutil/python/qgroup.c
@@ -103,39 +103,12 @@ static PyMethodDef QgroupInherit_methods[] = {
 
 PyTypeObject QgroupInherit_type = {
 	PyVarObject_HEAD_INIT(NULL, 0)
-	"btrfsutil.QgroupInherit",		/* tp_name */
-	sizeof(QgroupInherit),			/* tp_basicsize */
-	0,					/* tp_itemsize */
-	(destructor)QgroupInherit_dealloc,	/* tp_dealloc */
-	NULL,					/* tp_print */
-	NULL,					/* tp_getattr */
-	NULL,					/* tp_setattr */
-	NULL,					/* tp_as_async */
-	NULL,					/* tp_repr */
-	NULL,					/* tp_as_number */
-	NULL,					/* tp_as_sequence */
-	NULL,					/* tp_as_mapping */
-	NULL,					/* tp_hash  */
-	NULL,					/* tp_call */
-	NULL,					/* tp_str */
-	(getattrofunc)QgroupInherit_getattro,	/* tp_getattro */
-	NULL,					/* tp_setattro */
-	NULL,					/* tp_as_buffer */
-	Py_TPFLAGS_DEFAULT,			/* tp_flags */
-	QgroupInherit_DOC,			/* tp_doc */
-	NULL,					/* tp_traverse */
-	NULL,					/* tp_clear */
-	NULL,					/* tp_richcompare */
-	0,					/* tp_weaklistoffset */
-	NULL,					/* tp_iter */
-	NULL,					/* tp_iternext */
-	QgroupInherit_methods,			/* tp_methods */
-	NULL,					/* tp_members */
-	NULL,					/* tp_getset */
-	NULL,					/* tp_base */
-	NULL,					/* tp_dict */
-	NULL,					/* tp_descr_get */
-	NULL,					/* tp_descr_set */
-	0,					/* tp_dictoffset */
-	(initproc)QgroupInherit_init,		/* tp_init */
+	.tp_name		= "btrfsutil.QgroupInherit",
+	.tp_basicsize		= sizeof(QgroupInherit),
+	.tp_dealloc		= (destructor)QgroupInherit_dealloc,
+	.tp_getattro		= (getattrofunc)QgroupInherit_getattro,
+	.tp_flags		= Py_TPFLAGS_DEFAULT,
+	.tp_doc			= QgroupInherit_DOC,
+	.tp_methods		= QgroupInherit_methods,
+	.tp_init		= (initproc)QgroupInherit_init,
 };
-- 
2.24.0

[PATCH 4/4] btrfs-progs: libbtrfsutil: Convert to designated initialization for SubvolumeIterator_type

[Qu Wenruo]

[BUG]
When compiling btrfs-progs with libbtrfsutil on a python3.8 system, we
got the following warning:

  subvolume.c:636:2: warning: initialization of ‘long int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
    636 |  NULL,     /* tp_print */
        |  ^~~~
  subvolume.c:636:2: note: (near initialization for ‘SubvolumeIterator_type.tp_vectorcall_offset’)

[CAUSE]
C definition of PyTypeObject changed in python 3.8.
Now at the old tp_print, we have tp_vectorcall_offset.

So we got above warning.

[FIX]
C has designated initialization, which can assign values to each named
member, without hard coding to match the offset.
And all the other uninitialized values will be set to 0, so we can save
a lot of unneeded "= 0" or "= NULL" lines.

Just use that awesome feature to avoid any future breakage.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 libbtrfsutil/python/subvolume.c | 44 +++++++--------------------------
 1 file changed, 9 insertions(+), 35 deletions(-)

diff --git a/libbtrfsutil/python/subvolume.c b/libbtrfsutil/python/subvolume.c
index 0f893b9171fa..a837d2e32f36 100644
--- a/libbtrfsutil/python/subvolume.c
+++ b/libbtrfsutil/python/subvolume.c
@@ -629,39 +629,13 @@ static PyMethodDef SubvolumeIterator_methods[] = {
 
 PyTypeObject SubvolumeIterator_type = {
 	PyVarObject_HEAD_INIT(NULL, 0)
-	"btrfsutil.SubvolumeIterator",		/* tp_name */
-	sizeof(SubvolumeIterator),		/* tp_basicsize */
-	0,					/* tp_itemsize */
-	(destructor)SubvolumeIterator_dealloc,	/* tp_dealloc */
-	NULL,					/* tp_print */
-	NULL,					/* tp_getattr */
-	NULL,					/* tp_setattr */
-	NULL,					/* tp_as_async */
-	NULL,					/* tp_repr */
-	NULL,					/* tp_as_number */
-	NULL,					/* tp_as_sequence */
-	NULL,					/* tp_as_mapping */
-	NULL,					/* tp_hash  */
-	NULL,					/* tp_call */
-	NULL,					/* tp_str */
-	NULL,					/* tp_getattro */
-	NULL,					/* tp_setattro */
-	NULL,					/* tp_as_buffer */
-	Py_TPFLAGS_DEFAULT,			/* tp_flags */
-	SubvolumeIterator_DOC,			/* tp_doc */
-	NULL,					/* tp_traverse */
-	NULL,					/* tp_clear */
-	NULL,					/* tp_richcompare */
-	0,					/* tp_weaklistoffset */
-	PyObject_SelfIter,			/* tp_iter */
-	(iternextfunc)SubvolumeIterator_next,	/* tp_iternext */
-	SubvolumeIterator_methods,		/* tp_methods */
-	NULL,					/* tp_members */
-	NULL,					/* tp_getset */
-	NULL,					/* tp_base */
-	NULL,					/* tp_dict */
-	NULL,					/* tp_descr_get */
-	NULL,					/* tp_descr_set */
-	0,					/* tp_dictoffset */
-	(initproc)SubvolumeIterator_init,	/* tp_init */
+	.tp_name		= "btrfsutil.SubvolumeIterator",
+	.tp_basicsize		= sizeof(SubvolumeIterator),
+	.tp_dealloc		= (destructor)SubvolumeIterator_dealloc,
+	.tp_flags		= Py_TPFLAGS_DEFAULT,
+	.tp_doc			= SubvolumeIterator_DOC,
+	.tp_iter		= PyObject_SelfIter,
+	.tp_iternext		= (iternextfunc)SubvolumeIterator_next,
+	.tp_methods		= SubvolumeIterator_methods,
+	.tp_init		= (initproc)SubvolumeIterator_init,
 };
-- 
2.24.0

Re: [PATCH 0/4] btrfs-progs: Compiling warning fixes for devel branch

[Nikolay Borisov]

On 18.11.19 г. 8:30 ч., Qu Wenruo wrote:
> We have several compiling errors, in devel branch.
> One looks like a false alert from compiler, the first patch will
> workaround it.
> 
> 3 warning from libbtrfsutils are due to python3.8 changes.
> Handle it properly by using designated initialization, which also saves
> us quite some lines.
> 
> Qu Wenruo (4):
>   btrfs-progs: check/lowmem: Fix a false alert on uninitialized value
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     BtrfsUtilError_type
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     QgroupInherit_type
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     SubvolumeIterator_type
> 
>  check/mode-common.c             |  2 +-
>  libbtrfsutil/python/error.c     | 49 ++++++++-------------------------
>  libbtrfsutil/python/qgroup.c    | 43 ++++++-----------------------
>  libbtrfsutil/python/subvolume.c | 44 ++++++-----------------------
>  4 files changed, 30 insertions(+), 108 deletions(-)
> 

For the whole series:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 0/4] btrfs-progs: Compiling warning fixes for devel branch

[David Sterba]

On Mon, Nov 18, 2019 at 02:30:48PM +0800, Qu Wenruo wrote:
> We have several compiling errors, in devel branch.
> One looks like a false alert from compiler, the first patch will
> workaround it.
> 
> 3 warning from libbtrfsutils are due to python3.8 changes.
> Handle it properly by using designated initialization, which also saves
> us quite some lines.
> 
> Qu Wenruo (4):
>   btrfs-progs: check/lowmem: Fix a false alert on uninitialized value
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     BtrfsUtilError_type
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     QgroupInherit_type
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     SubvolumeIterator_type

Added to devel, thanks. Please note that libbtrfsutil does not need the
btrfs-progs prefix, as it's considered a separate part.

Re: [PATCH 0/4] btrfs-progs: Compiling warning fixes for devel branch

[Omar Sandoval]

On Mon, Nov 18, 2019 at 02:30:48PM +0800, Qu Wenruo wrote:
> We have several compiling errors, in devel branch.
> One looks like a false alert from compiler, the first patch will
> workaround it.
> 
> 3 warning from libbtrfsutils are due to python3.8 changes.
> Handle it properly by using designated initialization, which also saves
> us quite some lines.
> 
> Qu Wenruo (4):
>   btrfs-progs: check/lowmem: Fix a false alert on uninitialized value
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     BtrfsUtilError_type
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     QgroupInherit_type
>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>     SubvolumeIterator_type
> 
>  check/mode-common.c             |  2 +-
>  libbtrfsutil/python/error.c     | 49 ++++++++-------------------------
>  libbtrfsutil/python/qgroup.c    | 43 ++++++-----------------------
>  libbtrfsutil/python/subvolume.c | 44 ++++++-----------------------
>  4 files changed, 30 insertions(+), 108 deletions(-)

Thanks for fixing the libbtrfsutil parts. For some reason, the
convention for Python C extensions is to not use designated
initializers, but after this breakage it's definitely safer to use them.

I guess Dave already merged these, but FWIW,

Reviewed-by: Omar Sandoval <osandov@fb.com>

Re: [PATCH 0/4] btrfs-progs: Compiling warning fixes for devel branch

[Qu WenRuo]

On 2019/11/19 上午2:27, Omar Sandoval wrote:
> On Mon, Nov 18, 2019 at 02:30:48PM +0800, Qu Wenruo wrote:
>> We have several compiling errors, in devel branch.
>> One looks like a false alert from compiler, the first patch will
>> workaround it.
>>
>> 3 warning from libbtrfsutils are due to python3.8 changes.
>> Handle it properly by using designated initialization, which also saves
>> us quite some lines.
>>
>> Qu Wenruo (4):
>>   btrfs-progs: check/lowmem: Fix a false alert on uninitialized value
>>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>>     BtrfsUtilError_type
>>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>>     QgroupInherit_type
>>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
>>     SubvolumeIterator_type
>>
>>  check/mode-common.c             |  2 +-
>>  libbtrfsutil/python/error.c     | 49 ++++++++-------------------------
>>  libbtrfsutil/python/qgroup.c    | 43 ++++++-----------------------
>>  libbtrfsutil/python/subvolume.c | 44 ++++++-----------------------
>>  4 files changed, 30 insertions(+), 108 deletions(-)
> 
> Thanks for fixing the libbtrfsutil parts. For some reason, the
> convention for Python C extensions is to not use designated
> initializers, but after this breakage it's definitely safer to use them.
> 
> I guess Dave already merged these, but FWIW,
> 
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> 
A small question inspired by this bug, but not specific to btrfs.

Does this mean each big python upgrade makes all c-binding binary
incompatible? Or this is just a hiccup for this python3.8 release?

If it's the former case, that doesn't look correct to me...

Thanks,
Qu

Re: [PATCH 0/4] btrfs-progs: Compiling warning fixes for devel branch

[Omar Sandoval]

On Mon, Nov 18, 2019 at 11:47:41PM +0000, Qu WenRuo wrote:
> 
> 
> On 2019/11/19 上午2:27, Omar Sandoval wrote:
> > On Mon, Nov 18, 2019 at 02:30:48PM +0800, Qu Wenruo wrote:
> >> We have several compiling errors, in devel branch.
> >> One looks like a false alert from compiler, the first patch will
> >> workaround it.
> >>
> >> 3 warning from libbtrfsutils are due to python3.8 changes.
> >> Handle it properly by using designated initialization, which also saves
> >> us quite some lines.
> >>
> >> Qu Wenruo (4):
> >>   btrfs-progs: check/lowmem: Fix a false alert on uninitialized value
> >>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
> >>     BtrfsUtilError_type
> >>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
> >>     QgroupInherit_type
> >>   btrfs-progs: libbtrfsutil: Convert to designated initialization for
> >>     SubvolumeIterator_type
> >>
> >>  check/mode-common.c             |  2 +-
> >>  libbtrfsutil/python/error.c     | 49 ++++++++-------------------------
> >>  libbtrfsutil/python/qgroup.c    | 43 ++++++-----------------------
> >>  libbtrfsutil/python/subvolume.c | 44 ++++++-----------------------
> >>  4 files changed, 30 insertions(+), 108 deletions(-)
> > 
> > Thanks for fixing the libbtrfsutil parts. For some reason, the
> > convention for Python C extensions is to not use designated
> > initializers, but after this breakage it's definitely safer to use them.
> > 
> > I guess Dave already merged these, but FWIW,
> > 
> > Reviewed-by: Omar Sandoval <osandov@fb.com>
> > 
> A small question inspired by this bug, but not specific to btrfs.
> 
> Does this mean each big python upgrade makes all c-binding binary
> incompatible? Or this is just a hiccup for this python3.8 release?
> 
> If it's the former case, that doesn't look correct to me...

In general, each major Python version may change the C extension ABI,
which is why the Python version is encoded in the file name:

/usr/lib/python3.8/site-packages/btrfsutil.cpython-38-x86_64-linux-gnu.so

Usually they maintain source-level compatibility, but it appears that
CPython itself uses 0 for its unused initializers rather than NULL, so
they probably just missed this.