[libvirt] [PATCH 0/3] Couple of build fixes
I've noticed couple of build failures with a bleeding edge gcc. Here are fixes to some of them. Michal Privoznik (3): docs: Teach apibuild to deal with (1U << 31) too Turn 1<<31 into 1U<<31 build: use gnulib's unsetenv bootstrap.conf | 1 + docs/apibuild.py | 3 ++- include/libvirt/libvirt-domain.h | 2 +- src/util/virtypedparam.h | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) -- 2.8.3
The apibuild script is a terrifying beast that parses some source files of ours and produces an XML representation of them. When it comes to parsing enums we have in some header files, it tries to be clever and detect a value that an enum member has (or if it is an alias for a different member). Whilst doing that it has to deal with values we give to the members in many formats. At some places we just pass the value in decimal: VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1, in other places, we use the aliasing: VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = VIR_CONNECT_LIST_DOMAINS_ACTIVE, and in other places bitwise shifts are used: VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1 << 31, /* enforce requested stats */ The script tries to parse all of these resulting in the following tokens: "1", "VIR_CONNECT_LIST_DOMAINS_ACTIVE", "1<<31"; Then, the script tries to turn these into integers using python's eval() function. This function succeeds on the first and the last tokens. But, if we were to modify the last example so that it's of the following form: VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1U << 31, /* enforce requested stats */ the token representing enum's member value will then be "1U<<31". So our parsing is good. Unfortunately, python is not aware of the difference between signed and unsigned C types, therefore eval() fails over this token and the parser falls back thinking it's an alias to another enum member. Well it's not. The solution is to transform [0-9]U into [0-9] as for our purposes here it's the same thing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/apibuild.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 7e4a526..712b8b9 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -1398,7 +1398,8 @@ class CParser: token = self.token() while token[0] != "sep" or (token[1] != ',' and token[1] != '}'): - value = value + token[1] + # We might be dealing with '1U << 12' here + value = value + re.sub("(\d+)U","\\1", token[1]) token = self.token() else: try: -- 2.8.3
On Mon, May 30, 2016 at 09:31:00 +0200, Michal Privoznik wrote:
The apibuild script is a terrifying beast that parses some source files of ours and produces an XML representation of them. When it comes to parsing enums we have in some header files, it tries to be clever and detect a value that an enum member has (or if it is an alias for a different member). Whilst doing that it has to deal with values we give to the members in many formats. At some places we just pass the value in decimal:
VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1,
in other places, we use the aliasing:
VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = VIR_CONNECT_LIST_DOMAINS_ACTIVE,
and in other places bitwise shifts are used:
VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1 << 31, /* enforce requested stats */
The script tries to parse all of these resulting in the following tokens: "1", "VIR_CONNECT_LIST_DOMAINS_ACTIVE", "1<<31"; Then, the script tries to turn these into integers using python's eval() function. This function succeeds on the first and the last tokens. But, if we were to modify the last example so that it's of the following form:
VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1U << 31, /* enforce requested stats */
the token representing enum's member value will then be "1U<<31". So our parsing is good. Unfortunately, python is not aware of the difference between signed and unsigned C types, therefore eval() fails over this token and the parser falls back thinking it's an alias to another enum member. Well it's not.
The solution is to transform [0-9]U into [0-9] as for our purposes here it's the same thing.
I was worried that it might mangle aliases for other enums containing numbers followed by a capital U but I didn't manage to break it. ACK
On Mon, May 30, 2016 at 12:56:31 +0200, Peter Krempa wrote:
On Mon, May 30, 2016 at 09:31:00 +0200, Michal Privoznik wrote:
[...]
I was worried that it might mangle aliases for other enums containing numbers followed by a capital U but I didn't manage to break it.
Humm. I actually didn't apply your patch while trying it. So if you have a enum value like "VIR_TEST_123U_BLAH" it becomes "VIR_TEST_123_BLAH" in the XML file which is not good.
ACK
Thus I have to take back the ACK. Peter
On 30.05.2016 13:20, Peter Krempa wrote:
On Mon, May 30, 2016 at 12:56:31 +0200, Peter Krempa wrote:
On Mon, May 30, 2016 at 09:31:00 +0200, Michal Privoznik wrote:
[...]
I was worried that it might mangle aliases for other enums containing numbers followed by a capital U but I didn't manage to break it.
Humm. I actually didn't apply your patch while trying it.
So if you have a enum value like "VIR_TEST_123U_BLAH" it becomes "VIR_TEST_123_BLAH" in the XML file which is not good.
ACK
Thus I have to take back the ACK.
D'oh! I've just pushed the patch. I wonder if I can fix it. Hang on. Michal
Apparently, 1 << 31 is signed which in turn does not fit into a signed integer variable: ../../include/libvirt/libvirt-domain.h:1881:57: error: result of '1 << 31' requires 33 bits to represent, but 'int' only has 32 bits [-Werror=shift-overflow=] VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1 << 31, /* enforce requested stats */ ^~ cc1: all warnings being treated as errors The solution is to make it an unsigned value. I've found only two such occurrences in our code base. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 2 +- src/util/virtypedparam.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 27ed29a..cba4fa5 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1878,7 +1878,7 @@ typedef enum { VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER, VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING = 1 << 30, /* include backing chain for block stats */ - VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1 << 31, /* enforce requested stats */ + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1U << 31, /* enforce requested stats */ } virConnectGetAllDomainStatsFlags; int virConnectGetAllDomainStats(virConnectPtr conn, diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 73f710a..7ab143f 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -32,7 +32,7 @@ * Flag indicating that the params has multiple occurrences of the parameter. * Only used as a flag for @type argument of the virTypedParamsValidate. */ -# define VIR_TYPED_PARAM_MULTIPLE (1 << 31) +# define VIR_TYPED_PARAM_MULTIPLE (1U << 31) verify(!(VIR_TYPED_PARAM_LAST & VIR_TYPED_PARAM_MULTIPLE)); -- 2.8.3
Now that gnulib has lifted it's licensing of unsetenv, we should use it. Just like we use its counterpart - setenv, already. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- bootstrap.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/bootstrap.conf b/bootstrap.conf index 783b3a3..0db6b62 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -120,6 +120,7 @@ time_r timegm ttyname_r uname +unsetenv useless-if-before-free usleep vasprintf -- 2.8.3
participants (2)
-
Michal Privoznik -
Peter Krempa