6:10 p.m.
For printf("%*s",foo,bar), clang complains if foo is not int:
warning: field width should have type 'int', but argument has
type 'unsigned int' [-Wformat]
* src/conf/storage_encryption_conf.c
(virStorageEncryptionSecretFormat, virStorageEncryptionFormat):
Use correct type.
---
src/conf/storage_encryption_conf.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c
index 7a64050..8f29492 100644
--- a/src/conf/storage_encryption_conf.c
+++ b/src/conf/storage_encryption_conf.c
@@ -1,7 +1,7 @@
/*
* storage_encryption_conf.c: volume encryption information
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -216,7 +216,7 @@ virStorageEncryptionParseNode(xmlDocPtr xml, xmlNodePtr root)
static int
virStorageEncryptionSecretFormat(virBufferPtr buf,
virStorageEncryptionSecretPtr secret,
- unsigned int indent)
+ int indent)
{
const char *type;
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -249,14 +249,15 @@ virStorageEncryptionFormat(virBufferPtr buf,
return -1;
}
virBufferVSprintf(buf, "%*s<encryption format='%s'>\n",
- indent, "", format);
+ (int) indent, "", format);
for (i = 0; i < enc->nsecrets; i++) {
- if (virStorageEncryptionSecretFormat(buf, enc->secrets[i], indent + 2) <
0)
+ if (virStorageEncryptionSecretFormat(buf, enc->secrets[i],
+ indent + 2) < 0)
return -1;
}
- virBufferVSprintf(buf, "%*s</encryption>\n", indent, "");
+ virBufferVSprintf(buf, "%*s</encryption>\n", (int) indent,
"");
return 0;
}
--
1.6.6.1
6:35 p.m.
On Fri, May 07, 2010 at 05:10:12PM -0600, Eric Blake wrote:
For printf("%*s",foo,bar), clang complains if foo is not
int:
warning: field width should have type 'int', but argument has
type 'unsigned int' [-Wformat]
* src/conf/storage_encryption_conf.c
(virStorageEncryptionSecretFormat, virStorageEncryptionFormat):
Use correct type.
---
src/conf/storage_encryption_conf.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c
index 7a64050..8f29492 100644
--- a/src/conf/storage_encryption_conf.c
+++ b/src/conf/storage_encryption_conf.c
@@ -1,7 +1,7 @@
/*
* storage_encryption_conf.c: volume encryption information
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -216,7 +216,7 @@ virStorageEncryptionParseNode(xmlDocPtr xml, xmlNodePtr root)
static int
virStorageEncryptionSecretFormat(virBufferPtr buf,
virStorageEncryptionSecretPtr secret,
- unsigned int indent)
+ int indent)
{
const char *type;
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -249,14 +249,15 @@ virStorageEncryptionFormat(virBufferPtr buf,
return -1;
}
virBufferVSprintf(buf, "%*s<encryption format='%s'>\n",
- indent, "", format);
+ (int) indent, "", format);
for (i = 0; i < enc->nsecrets; i++) {
- if (virStorageEncryptionSecretFormat(buf, enc->secrets[i], indent + 2) <
0)
+ if (virStorageEncryptionSecretFormat(buf, enc->secrets[i],
+ indent + 2) < 0)
return -1;
}
- virBufferVSprintf(buf, "%*s</encryption>\n", indent, "");
+ virBufferVSprintf(buf, "%*s</encryption>\n", (int) indent,
"");
return 0;
}
--
1.6.6.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
What's the need for the cast if the function parameter is changed to
int instead of unsigned int? The two callers pass in hardcoded
values, so I'd think just changing the param would silence the
warning, wouldn't it?
Dave
12:58 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
For printf("%*s",foo,bar), clang complains if foo is not int:
warning: field width should have type 'int', but argument has
type 'unsigned int' [-Wformat]
* src/conf/storage_encryption_conf.c
(virStorageEncryptionSecretFormat, virStorageEncryptionFormat):
Use correct type.
* src/conf/storage_encryption_conf.h (virStorageEncryptionFormat):
Likewise.
---
> + virBufferVSprintf(buf,
"%*s</encryption>\n", (int) indent, "");
>
What's the need for the cast if the function parameter is changed to
int instead of unsigned int? The two callers pass in hardcoded
values, so I'd think just changing the param would silence the
warning, wouldn't it?
Sure - it's just one more file to touch. Change from v1: update
the .h file to use fewer casts.
src/conf/storage_encryption_conf.c | 9 +++++----
src/conf/storage_encryption_conf.h | 4 ++--
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c
index 7a64050..7bbdbc1 100644
--- a/src/conf/storage_encryption_conf.c
+++ b/src/conf/storage_encryption_conf.c
@@ -1,7 +1,7 @@
/*
* storage_encryption_conf.c: volume encryption information
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -216,7 +216,7 @@ virStorageEncryptionParseNode(xmlDocPtr xml, xmlNodePtr root)
static int
virStorageEncryptionSecretFormat(virBufferPtr buf,
virStorageEncryptionSecretPtr secret,
- unsigned int indent)
+ int indent)
{
const char *type;
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -237,7 +237,7 @@ virStorageEncryptionSecretFormat(virBufferPtr buf,
int
virStorageEncryptionFormat(virBufferPtr buf,
virStorageEncryptionPtr enc,
- unsigned int indent)
+ int indent)
{
const char *format;
size_t i;
@@ -252,7 +252,8 @@ virStorageEncryptionFormat(virBufferPtr buf,
indent, "", format);
for (i = 0; i < enc->nsecrets; i++) {
- if (virStorageEncryptionSecretFormat(buf, enc->secrets[i], indent + 2) <
0)
+ if (virStorageEncryptionSecretFormat(buf, enc->secrets[i],
+ indent + 2) < 0)
return -1;
}
diff --git a/src/conf/storage_encryption_conf.h b/src/conf/storage_encryption_conf.h
index 8309255..c722cc6 100644
--- a/src/conf/storage_encryption_conf.h
+++ b/src/conf/storage_encryption_conf.h
@@ -1,7 +1,7 @@
/*
* storage_encryption_conf.h: volume encryption information
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -68,7 +68,7 @@ virStorageEncryptionPtr virStorageEncryptionParseNode(xmlDocPtr xml,
xmlNodePtr root);
int virStorageEncryptionFormat(virBufferPtr buf,
virStorageEncryptionPtr enc,
- unsigned int indent);
+ int indent);
/* A helper for VIR_STORAGE_ENCRYPTION_FORMAT_QCOW */
enum {
--
1.7.0.1
1:37 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
On Mon, May 10, 2010 at 11:58:26AM -0600, Eric Blake wrote:
For printf("%*s",foo,bar), clang complains if foo is not
int:
warning: field width should have type 'int', but argument has
type 'unsigned int' [-Wformat]
* src/conf/storage_encryption_conf.c
(virStorageEncryptionSecretFormat, virStorageEncryptionFormat):
Use correct type.
* src/conf/storage_encryption_conf.h (virStorageEncryptionFormat):
Likewise.
---
> > + virBufferVSprintf(buf, "%*s</encryption>\n", (int) indent,
"");
> >
> What's the need for the cast if the function parameter is changed to
> int instead of unsigned int? The two callers pass in hardcoded
> values, so I'd think just changing the param would silence the
> warning, wouldn't it?
Sure - it's just one more file to touch. Change from v1: update
the .h file to use fewer casts.
src/conf/storage_encryption_conf.c | 9 +++++----
src/conf/storage_encryption_conf.h | 4 ++--
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c
index 7a64050..7bbdbc1 100644
--- a/src/conf/storage_encryption_conf.c
+++ b/src/conf/storage_encryption_conf.c
@@ -1,7 +1,7 @@
/*
* storage_encryption_conf.c: volume encryption information
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -216,7 +216,7 @@ virStorageEncryptionParseNode(xmlDocPtr xml, xmlNodePtr root)
static int
virStorageEncryptionSecretFormat(virBufferPtr buf,
virStorageEncryptionSecretPtr secret,
- unsigned int indent)
+ int indent)
{
const char *type;
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -237,7 +237,7 @@ virStorageEncryptionSecretFormat(virBufferPtr buf,
int
virStorageEncryptionFormat(virBufferPtr buf,
virStorageEncryptionPtr enc,
- unsigned int indent)
+ int indent)
{
const char *format;
size_t i;
@@ -252,7 +252,8 @@ virStorageEncryptionFormat(virBufferPtr buf,
indent, "", format);
for (i = 0; i < enc->nsecrets; i++) {
- if (virStorageEncryptionSecretFormat(buf, enc->secrets[i], indent + 2) <
0)
+ if (virStorageEncryptionSecretFormat(buf, enc->secrets[i],
+ indent + 2) < 0)
return -1;
}
diff --git a/src/conf/storage_encryption_conf.h b/src/conf/storage_encryption_conf.h
index 8309255..c722cc6 100644
--- a/src/conf/storage_encryption_conf.h
+++ b/src/conf/storage_encryption_conf.h
@@ -1,7 +1,7 @@
/*
* storage_encryption_conf.h: volume encryption information
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -68,7 +68,7 @@ virStorageEncryptionPtr virStorageEncryptionParseNode(xmlDocPtr xml,
xmlNodePtr root);
int virStorageEncryptionFormat(virBufferPtr buf,
virStorageEncryptionPtr enc,
- unsigned int indent);
+ int indent);
/* A helper for VIR_STORAGE_ENCRYPTION_FORMAT_QCOW */
enum {
--
1.7.0.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
ACK
Dave
2:02 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
Eric Blake wrote:
For printf("%*s",foo,bar), clang complains if foo is not
int:
warning: field width should have type 'int', but argument has
type 'unsigned int' [-Wformat]
* src/conf/storage_encryption_conf.c
(virStorageEncryptionSecretFormat, virStorageEncryptionFormat):
Use correct type.
* src/conf/storage_encryption_conf.h (virStorageEncryptionFormat):
Likewise.
---
> > + virBufferVSprintf(buf, "%*s</encryption>\n", (int) indent,
"");
> >
> What's the need for the cast if the function parameter is changed to
> int instead of unsigned int? The two callers pass in hardcoded
> values, so I'd think just changing the param would silence the
> warning, wouldn't it?
Sure - it's just one more file to touch. Change from v1: update
the .h file to use fewer casts.
src/conf/storage_encryption_conf.c | 9 +++++----
src/conf/storage_encryption_conf.h | 4 ++--
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c
index 7a64050..7bbdbc1 100644
--- a/src/conf/storage_encryption_conf.c
+++ b/src/conf/storage_encryption_conf.c
@@ -1,7 +1,7 @@
/*
* storage_encryption_conf.c: volume encryption information
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -216,7 +216,7 @@ virStorageEncryptionParseNode(xmlDocPtr xml, xmlNodePtr root)
static int
virStorageEncryptionSecretFormat(virBufferPtr buf,
virStorageEncryptionSecretPtr secret,
- unsigned int indent)
+ int indent)
"unsigned int" sounds like the right type to me, since
"indent" never goes negative. And using the unsigned type
seems to be in line with policy in HACKING:
If a variable is counting something, be sure to declare it with an
unsigned type.
Of course, avoiding casts is good, too, but IMHO, not if
it makes us obfuscate (even ever so slightly) the types we use.
{
const char *type;
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -237,7 +237,7 @@ virStorageEncryptionSecretFormat(virBufferPtr buf,
int
virStorageEncryptionFormat(virBufferPtr buf,
virStorageEncryptionPtr enc,
- unsigned int indent)
+ int indent)
3:12 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
On 05/10/2010 01:02 PM, Jim Meyering wrote:
Eric Blake wrote:
> For printf("%*s",foo,bar), clang complains if foo is not int:
>
> warning: field width should have type 'int', but argument has
> type 'unsigned int' [-Wformat]
>
> virStorageEncryptionSecretFormat(virBufferPtr buf,
> virStorageEncryptionSecretPtr secret,
> - unsigned int indent)
> + int indent)
"unsigned int" sounds like the right type to me, since
"indent" never goes negative. And using the unsigned type
seems to be in line with policy in HACKING:
If a variable is counting something, be sure to declare it with an
unsigned type.
But printf("%*s", indent, string) would indeed behave differently if
indent goes negative (if signed) or larger than INT_MAX (if unsigned),
so clang's warning is realistic, and worth silencing in our quest to get
a clean clang build. About the only other thing I could think of to do
that would avoid confusion and also to avoid casts is to keep the public
interface with unsigned int, then add an intermediate helper variable:
int real_indent = indent;
and use real_indent in the printf call, but that seems like overkill.
Of course, avoiding casts is good, too, but IMHO, not if
it makes us obfuscate (even ever so slightly) the types we use.
Well, the real point of this patch was to silence a compiler warning
(not that we'd ever planning on passing an indent > 2G). Given Dave's
ACK, I went ahead and applied v2 as proposed, even if it does slightly
obfuscate the usage.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:32 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
Eric Blake wrote:
On 05/10/2010 01:02 PM, Jim Meyering wrote:
> Eric Blake wrote:
>> For printf("%*s",foo,bar), clang complains if foo is not int:
>>
>> warning: field width should have type 'int', but argument has
>> type 'unsigned int' [-Wformat]
>>
>> virStorageEncryptionSecretFormat(virBufferPtr buf,
>> virStorageEncryptionSecretPtr secret,
>> - unsigned int indent)
>> + int indent)
>
> "unsigned int" sounds like the right type to me, since
> "indent" never goes negative. And using the unsigned type
> seems to be in line with policy in HACKING:
>
> If a variable is counting something, be sure to declare it with an
> unsigned type.
I'm not lobbying for further change, but merely
making sure you see where I'm coming from.
But printf("%*s", indent, string) would indeed behave
differently if
indent goes negative (if signed) or larger than INT_MAX (if unsigned),
That's true, but the meaning of the variable's type is important
and affects two interfaces, whereas the cast is local.
Making it signed via "int" should make the reader/reviewer think
a negative value is an option. Here, it is not.
so clang's warning is realistic, and worth silencing in our quest
to get
No argument there.
My point was that a cast keeps the ugly compromise local to the
unfortunate interface (printf field width) that it accommodates,
rather than propagating it to another interface and even another file.
a clean clang build. About the only other thing I could think of to
do
that would avoid confusion and also to avoid casts is to keep the public
interface with unsigned int, then add an intermediate helper variable:
int real_indent = indent;
and use real_indent in the printf call, but that seems like overkill.
Using the cast is the lesser evil ;)
>
> Of course, avoiding casts is good, too, but IMHO, not if
> it makes us obfuscate (even ever so slightly) the types we use.
Well, the real point of this patch was to silence a compiler warning
(not that we'd ever planning on passing an indent > 2G). Given Dave's
ACK, I went ahead and applied v2 as proposed, even if it does slightly
obfuscate the usage.
Warning removal is important.
But it can end up being counterproductive if not done carefully.
3:33 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
On 05/10/2010 02:32 PM, Jim Meyering wrote:
Using the cast is the lesser evil ;)
>>
>> Of course, avoiding casts is good, too, but IMHO, not if
>> it makes us obfuscate (even ever so slightly) the types we use.
>
> Well, the real point of this patch was to silence a compiler warning
> (not that we'd ever planning on passing an indent > 2G). Given Dave's
> ACK, I went ahead and applied v2 as proposed, even if it does slightly
> obfuscate the usage.
Warning removal is important.
But it can end up being counterproductive if not done carefully.
Should I go ahead and revert v2, going back to v1 which localized the
change to just one file?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:38 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
Eric Blake wrote:
On 05/10/2010 02:32 PM, Jim Meyering wrote:
>
> Using the cast is the lesser evil ;)
>
>>>
>>> Of course, avoiding casts is good, too, but IMHO, not if
>>> it makes us obfuscate (even ever so slightly) the types we use.
>>
>> Well, the real point of this patch was to silence a compiler warning
>> (not that we'd ever planning on passing an indent > 2G). Given
Dave's
>> ACK, I went ahead and applied v2 as proposed, even if it does slightly
>> obfuscate the usage.
>
> Warning removal is important.
> But it can end up being counterproductive if not done carefully.
Should I go ahead and revert v2, going back to v1 which localized the
change to just one file?
This variable is not important.
It's the principle, as I tried to explain:
I'm not lobbying for further change, but merely
making sure you see where I'm coming from.
10:35 a.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
On Mon, May 10, 2010 at 02:12:29PM -0600, Eric Blake wrote:
On 05/10/2010 01:02 PM, Jim Meyering wrote:
> Eric Blake wrote:
>> For printf("%*s",foo,bar), clang complains if foo is not int:
>>
>> warning: field width should have type 'int', but argument has
>> type 'unsigned int' [-Wformat]
>>
>> virStorageEncryptionSecretFormat(virBufferPtr buf,
>> virStorageEncryptionSecretPtr secret,
>> - unsigned int indent)
>> + int indent)
>
> "unsigned int" sounds like the right type to me, since
> "indent" never goes negative. And using the unsigned type
> seems to be in line with policy in HACKING:
>
> If a variable is counting something, be sure to declare it with an
> unsigned type.
But printf("%*s", indent, string) would indeed behave differently if
indent goes negative (if signed) or larger than INT_MAX (if unsigned),
so clang's warning is realistic, and worth silencing in our quest to get
a clean clang build. About the only other thing I could think of to do
that would avoid confusion and also to avoid casts is to keep the public
interface with unsigned int, then add an intermediate helper variable:
int real_indent = indent;
and use real_indent in the printf call, but that seems like overkill.
>
> Of course, avoiding casts is good, too, but IMHO, not if
> it makes us obfuscate (even ever so slightly) the types we use.
Well, the real point of this patch was to silence a compiler warning
(not that we'd ever planning on passing an indent > 2G). Given Dave's
ACK, I went ahead and applied v2 as proposed, even if it does slightly
obfuscate the usage.
It's valid to pass a negative value, so int is the correct type. My
bad for using unsigned int in the first place.
From man printf:
A negative field width is taken as a '-' flag followed by a positive
field width.
Dave
12:20 p.m.
New subject: [libvirt] [PATCHv2] storage_encryption: silence clang warning
Dave Allan wrote:
...
It's valid to pass a negative value, so int is the correct type.
My
bad for using unsigned int in the first place.
>From man printf:
A negative field width is taken as a '-' flag followed by a positive
field width.
Hi Dave,
While it's true that a negative printf field width has meaning,
this code certainly does not try to take advantage of that.
The *meaning* (as it's used) of that variable is "a non-negative
indentation width". Hence, the fact that we've chosen a printf-based
implementation to output those spaces is just a detail, and should
not influence the type of the variable. Nor should it influence the
interfaces through which it is passed. We could easily switch to a
simple loop, in which case it'd be obvious there's no point in
accepting negative values.
The types used in variables and interfaces should be independent
of such implementation details.
This horse is now officially dead.
5344
days inactive
5348
days old
10 comments
3 participants
participants (3)
-
Dave Allan -
Eric Blake -
Jim Meyering