[libvirt] [PATCH] 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.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
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@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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ACK Dave
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)
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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.
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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.
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
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.
participants (3)
-
Dave Allan -
Eric Blake -
Jim Meyering