[libvirt] [PATCH 0/4] Resolve const correctness isues
Okay, okay. The approach in 4/4 can be considered hackish, but hey - it works! Michal Privoznik (4): Fix const correctness viralloc: Honor const correctness in VIR_FREE VIR_FREE: Avoid doing side work in callees virFree: Check const correctness src/conf/network_conf.c | 12 ++++++++---- src/locking/lock_driver_lockd.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/remote/remote_driver.c | 2 +- src/util/viralloc.c | 6 ++++-- src/util/viralloc.h | 33 ++++++++++++++++----------------- src/xenapi/xenapi_utils.c | 3 ++- tools/virsh-domain.c | 4 ++-- tools/wireshark/src/packet-libvirt.c | 6 +++--- tools/wireshark/src/packet-libvirt.h | 4 ++-- 10 files changed, 40 insertions(+), 34 deletions(-) -- 1.8.5.5
In many places we define a variable as a 'const char *' when in fact we modify it just a few lines below. Or even free it. We should not do that. There's one exception though, in xenSessionFree() xenapi_utils.c. We are freeing the xen_session structure which is defined in xen/api/xen_common.h public header. The structure contains session_id which is type of 'const char *' when in fact it should have been just 'char *'. So I'm leaving this unmodified, just noticing the fact in comment. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_lockd.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/remote/remote_driver.c | 2 +- src/xenapi/xenapi_utils.c | 1 + tools/virsh-domain.c | 4 ++-- tools/wireshark/src/packet-libvirt.c | 6 +++--- tools/wireshark/src/packet-libvirt.h | 4 ++-- 7 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 1ca7772..367d0ce 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -243,7 +243,7 @@ static virNetClientPtr virLockManagerLockDaemonConnectionNew(bool privileged, { virNetClientPtr client = NULL; char *lockdpath; - const char *daemonPath = NULL; + char *daemonPath = NULL; *prog = NULL; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 37e0588..8271e28 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2652,7 +2652,7 @@ static int virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *xml = NULL; + char *xml = NULL; int ret = -1; size_t i; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9d8120f..9a1d78f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -582,7 +582,7 @@ doRemoteOpen(virConnectPtr conn, trans_tcp, } transport; #ifndef WIN32 - const char *daemonPath = NULL; + char *daemonPath = NULL; #endif /* We handle *ALL* URIs here. The caller has rejected any diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 8b28914..a80d136 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -49,6 +49,7 @@ xenSessionFree(xen_session *session) VIR_FREE(session->error_description[i]); VIR_FREE(session->error_description); } + /* The session_id member is type of 'const char *'. Sigh. */ VIR_FREE(session->session_id); VIR_FREE(session); } diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5a17aff..ba47258 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10349,8 +10349,8 @@ vshPrepareDiskXML(xmlNodePtr disk_node, int type) { xmlNodePtr cur = NULL; - const char *disk_type = NULL; - const char *device_type = NULL; + char *disk_type = NULL; + char *device_type = NULL; xmlNodePtr new_node = NULL; char *ret = NULL; diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c index 0fe5f03..5c3c369 100644 --- a/tools/wireshark/src/packet-libvirt.c +++ b/tools/wireshark/src/packet-libvirt.c @@ -105,7 +105,7 @@ dissect_xdr_string(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, } } -static gchar * +static const gchar * format_xdr_bytes(guint8 *bytes, guint32 length) { gchar *buf; @@ -206,7 +206,7 @@ dissect_xdr_iterable(tvbuff_t *tvb, proto_item *ti, XDR *xdrs, gint ett, int rhf static gboolean dissect_xdr_vector(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 size, vir_xdr_dissector_t dissect) + int rhf, const gchar *rtype, guint32 size, vir_xdr_dissector_t dissect) { goffset start; proto_item *ti; @@ -219,7 +219,7 @@ dissect_xdr_vector(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, static gboolean dissect_xdr_array(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dissect) + int rhf, const gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dissect) { goffset start; proto_item *ti; diff --git a/tools/wireshark/src/packet-libvirt.h b/tools/wireshark/src/packet-libvirt.h index af54407..5f99fdf 100644 --- a/tools/wireshark/src/packet-libvirt.h +++ b/tools/wireshark/src/packet-libvirt.h @@ -105,9 +105,9 @@ static gboolean dissect_xdr_bytes(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, in static gboolean dissect_xdr_pointer(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, vir_xdr_dissector_t dp); static gboolean dissect_xdr_vector(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 size, vir_xdr_dissector_t dp); + int rhf, const gchar *rtype, guint32 size, vir_xdr_dissector_t dp); static gboolean dissect_xdr_array(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dp); + int rhf, const gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dp); # include "libvirt/protocol.h" -- 1.8.5.5
On 07/15/2014 06:38 AM, Michal Privoznik wrote:
In many places we define a variable as a 'const char *' when in fact we modify it just a few lines below. Or even free it. We should not do that.
There's one exception though, in xenSessionFree() xenapi_utils.c. We are freeing the xen_session structure which is defined in xen/api/xen_common.h public header. The structure contains session_id which is type of 'const char *' when in fact it should have been just 'char *'. So I'm leaving this unmodified, just noticing the fact in comment.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_lockd.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/remote/remote_driver.c | 2 +- src/xenapi/xenapi_utils.c | 1 + tools/virsh-domain.c | 4 ++-- tools/wireshark/src/packet-libvirt.c | 6 +++--- tools/wireshark/src/packet-libvirt.h | 4 ++-- 7 files changed, 11 insertions(+), 10 deletions(-)
ACK; safe to apply this one even while waiting for me to propose a v2 for the rest of the series.
+++ b/src/xenapi/xenapi_utils.c @@ -49,6 +49,7 @@ xenSessionFree(xen_session *session) VIR_FREE(session->error_description[i]); VIR_FREE(session->error_description); } + /* The session_id member is type of 'const char *'. Sigh. */ VIR_FREE(session->session_id); VIR_FREE(session);
By the way, once VIR_FREE is fixed, we can work around this instance by use of a temporary variable: /* cast away bogus const from xen header */ char *tmp = (char *)session->session_id; VIR_FREE(tmp); VIR_FREE(session);
+++ b/tools/wireshark/src/packet-libvirt.c @@ -105,7 +105,7 @@ dissect_xdr_string(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, } }
-static gchar * +static const gchar *
Side note - gchar is a pointless type. But not our fault that wireshark is using it instead of plain char. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Since we've corrected all the callers that passed a const pointer to VIR_FREE() we may drop the typecasting horribility and let the macro be a simple wrapper over the virFree() function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viralloc.h | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 7125e67..71b4a45 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -547,20 +547,7 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * * This macro is safe to use on arguments with side effects. */ -# if !STATIC_ANALYSIS -/* The ternary ensures that ptr is a pointer and not an integer type, - * while evaluating ptr only once. This gives us extra compiler - * safety when compiling under gcc. For now, we intentionally cast - * away const, since a number of callers safely pass const char *. - */ -# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr))) -# else -/* The Coverity static analyzer considers the else path of the "?:" and - * flags the VIR_FREE() of the address of the address of memory as a - * RESOURCE_LEAK resulting in numerous false positives (eg, VIR_FREE(&ptr)) - */ -# define VIR_FREE(ptr) virFree((void *) &(ptr)) -# endif +# define VIR_FREE(ptr) virFree(&(ptr)) void virAllocTestInit(void); int virAllocTestCount(void); -- 1.8.5.5
On 07/15/2014 06:38 AM, Michal Privoznik wrote:
Since we've corrected all the callers that passed a const pointer to VIR_FREE() we may drop the typecasting horribility and let the macro be a simple wrapper over the virFree() function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viralloc.h | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
NACK. I agree that we want to fix the macro to not cast away const, but you simplified it too far.
diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 7125e67..71b4a45 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -547,20 +547,7 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * * This macro is safe to use on arguments with side effects. */ -# if !STATIC_ANALYSIS -/* The ternary ensures that ptr is a pointer and not an integer type, - * while evaluating ptr only once. This gives us extra compiler - * safety when compiling under gcc. For now, we intentionally cast - * away const, since a number of callers safely pass const char *. - */ -# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr)))
This definition was doing two things - casting away const, AND doing type validation. virFree(&foo) will compile even if foo is an int, but we want to ensure that foo is of type char*. I think what you want is: # define VIR_FREE(ptr) virFree(1 ? &(ptr) : (ptr))
-# else -/* The Coverity static analyzer considers the else path of the "?:" and - * flags the VIR_FREE() of the address of the address of memory as a - * RESOURCE_LEAK resulting in numerous false positives (eg, VIR_FREE(&ptr)) - */ -# define VIR_FREE(ptr) virFree((void *) &(ptr))
and keep this alternative to hush up coverity. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/15/2014 09:41 AM, Eric Blake wrote: <...snip...>
This definition was doing two things - casting away const, AND doing type validation. virFree(&foo) will compile even if foo is an int, but we want to ensure that foo is of type char*.
I think what you want is:
# define VIR_FREE(ptr) virFree(1 ? &(ptr) : (ptr))
-# else -/* The Coverity static analyzer considers the else path of the "?:" and - * flags the VIR_FREE() of the address of the address of memory as a - * RESOURCE_LEAK resulting in numerous false positives (eg, VIR_FREE(&ptr)) - */ -# define VIR_FREE(ptr) virFree((void *) &(ptr))
and keep this alternative to hush up coverity.
FWIW: I got notice a couple weeks ago that Coverity 7.5.0 (released June 27, 2014) has a fix for this issue. Since there's no "guarantee" that everyone who runs a Coverity scan will update to that new version - I'd be hesitant to say that we could just do away with it since without this "condition"- Coverity will be very noisy. John
There are just two places where we rely on the fact that VIR_FREE() macro is without any side effects. In the future, this property of the macro is going to change, so we need the code to be adjusted to deal with argument passed to VIR_FREE() being evaluated multiple times. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ce4d4d8..d60a60e 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -180,8 +180,10 @@ virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) static void virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def) { - while (def->nnames) - VIR_FREE(def->names[--def->nnames]); + size_t i; + + for (i = 0; i < def->nnames; i++) + VIR_FREE(def->names[i]); VIR_FREE(def->names); } @@ -197,9 +199,11 @@ virNetworkDNSSrvDefClear(virNetworkDNSSrvDefPtr def) static void virNetworkDNSDefClear(virNetworkDNSDefPtr def) { + size_t i; + if (def->forwarders) { - while (def->nfwds) - VIR_FREE(def->forwarders[--def->nfwds]); + for (i = 0; i < def->nfwds; i++) + VIR_FREE(def->forwarders[i]); VIR_FREE(def->forwarders); } if (def->txts) { -- 1.8.5.5
On 07/15/2014 02:38 PM, Michal Privoznik wrote:
There are just two places where we rely on the fact that VIR_FREE() macro is without any side effects. In the future, this property of the macro is going to change, so we need the code to be adjusted to deal with argument passed to VIR_FREE() being evaluated multiple times.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
NACK, this completely removes the side effect. You need to adjust the callers for that. IMO we should set n<elems> to 0 in all *Clear functions, not just virNetworkDNSHostDefClear, after which NULL might be dereferenced based on the n<elems> variable. Jan
On 15.07.2014 14:58, Ján Tomko wrote:
On 07/15/2014 02:38 PM, Michal Privoznik wrote:
There are just two places where we rely on the fact that VIR_FREE() macro is without any side effects. In the future, this property of the macro is going to change, so we need the code to be adjusted to deal with argument passed to VIR_FREE() being evaluated multiple times.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
NACK, this completely removes the side effect. You need to adjust the callers for that.
Well, the arrays that n<elems> refer to are freed immediately, but I agree that we should keep the code clean.
IMO we should set n<elems> to 0 in all *Clear functions, not just virNetworkDNSHostDefClear, after which NULL might be dereferenced based on the n<elems> variable.
I don't see how that could be possible with current code. The virNetworkDNSHostDefClear is called only on cleanup paths where accessing def->names or def->forwarders is not possible anymore.
Jan
Okay, consider this squashed in: diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d60a60e..dcb521f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -184,6 +184,7 @@ virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def) for (i = 0; i < def->nnames; i++) VIR_FREE(def->names[i]); + def->nnames = 0; VIR_FREE(def->names); } @@ -204,6 +205,7 @@ virNetworkDNSDefClear(virNetworkDNSDefPtr def) if (def->forwarders) { for (i = 0; i < def->nfwds; i++) VIR_FREE(def->forwarders[i]); + def->nfwds = 0; VIR_FREE(def->forwarders); } if (def->txts) { Michal
On 07/15/2014 03:38 PM, Michal Privoznik wrote:
On 15.07.2014 14:58, Ján Tomko wrote:
On 07/15/2014 02:38 PM, Michal Privoznik wrote: IMO we should set n<elems> to 0 in all *Clear functions, not just virNetworkDNSHostDefClear, after which NULL might be dereferenced based on the n<elems> variable.
I don't see how that could be possible with current code. The virNetworkDNSHostDefClear is called only on cleanup paths where accessing def->names or def->forwarders is not possible anymore.
In virNetworkDefUpdateDNSHost when parsing in virNetworkDNSHostDefParseXML fails, DNSHostDefClear is called in both functions. Jan
On 07/15/2014 06:38 AM, Michal Privoznik wrote:
There are just two places where we rely on the fact that VIR_FREE() macro is without any side effects. In the future, this property of the macro is going to change, so we need the code to be adjusted to deal with argument passed to VIR_FREE() being evaluated multiple times.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
NACK; my v2 keeps the property, so we don't need to work around it changing after all. https://www.redhat.com/archives/libvir-list/2014-July/msg00760.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Up to now it's possible to do something like this: const char *ptr; ptr = strdup("my example string"); VIR_FREE(ptr); The problem is, const char * pointers should not be modified (and freeing them is kind of modification). We should avoid this. A little trick is used: assigning a const pointer into 'void *' triggers compiler warning about discarding 'const' qualifier from pointer. So the virFree() function gains new dummy argument, that is not touched anyhow, just fulfills the const correctness check duty. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viralloc.c | 6 ++++-- src/util/viralloc.h | 20 ++++++++++++++++---- src/xenapi/xenapi_utils.c | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/util/viralloc.c b/src/util/viralloc.c index be9f0fe..0134e67 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -372,7 +372,7 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) ignore_value(virReallocN(ptrptr, size, *countptr -= toremove, false, 0, NULL, NULL, 0)); else { - virFree(ptrptr); + virFree(NULL, ptrptr); *countptr = 0; } } @@ -569,13 +569,15 @@ int virAllocVar(void *ptrptr, /** * virFree: + * @ptr: dummy pointer to check const correctness * @ptrptr: pointer to pointer for address of memory to be freed * * Release the chunk of memory in the pointer pointed to by * the 'ptrptr' variable. After release, 'ptrptr' will be * updated to point to NULL. */ -void virFree(void *ptrptr) +void virFree(void *ptr ATTRIBUTE_UNUSED, + void *ptrptr) { int save_errno = errno; diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 71b4a45..8fbe56f 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -76,7 +76,7 @@ int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t co bool report, int domcode, const char *filename, const char *funcname, size_t linenr) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); +void virFree(void *ptr, void *ptrptr) ATTRIBUTE_NONNULL(2); /** * VIR_ALLOC: @@ -543,11 +543,23 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * @ptr: pointer holding address to be freed * * Free the memory stored in 'ptr' and update to point - * to NULL. + * to NULL. Moreover, this macro has a side effect in + * form of evaluating passed argument multiple times. + * But advantage is, it checks the cont correctness + * (that you are not freeing a const pointer). + */ +# define VIR_FREE(ptr) virFree(ptr, &(ptr)) + +/** + * VIR_FREE_BROKEN: + * @ptr: pointer holding address to be freed * - * This macro is safe to use on arguments with side effects. + * Twin macro of VIR_FREE. While it does evaluate + * argument only once, it does not check const + * correctness and therefore you want to use it if and + * only if necessary. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE_BROKEN(ptr) virFree(NULL, &(ptr)) void virAllocTestInit(void); int virAllocTestCount(void); diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index a80d136..db555d2 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -50,7 +50,7 @@ xenSessionFree(xen_session *session) VIR_FREE(session->error_description); } /* The session_id member is type of 'const char *'. Sigh. */ - VIR_FREE(session->session_id); + VIR_FREE_BROKEN(session->session_id); VIR_FREE(session); } -- 1.8.5.5
On Tue, Jul 15, 2014 at 02:38:36PM +0200, Michal Privoznik wrote:
Up to now it's possible to do something like this:
const char *ptr;
ptr = strdup("my example string");
VIR_FREE(ptr);
The problem is, const char * pointers should not be modified (and freeing them is kind of modification). We should avoid this. A little trick is used: assigning a const pointer into 'void *' triggers compiler warning about discarding 'const' qualifier from pointer. So the virFree() function gains new dummy argument, that is not touched anyhow, just fulfills the const correctness check duty.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viralloc.c | 6 ++++-- src/util/viralloc.h | 20 ++++++++++++++++---- src/xenapi/xenapi_utils.c | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/util/viralloc.c b/src/util/viralloc.c index be9f0fe..0134e67 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c [...] @@ -569,13 +569,15 @@ int virAllocVar(void *ptrptr,
/** * virFree: + * @ptr: dummy pointer to check const correctness * @ptrptr: pointer to pointer for address of memory to be freed * * Release the chunk of memory in the pointer pointed to by * the 'ptrptr' variable. After release, 'ptrptr' will be * updated to point to NULL. */ -void virFree(void *ptrptr) +void virFree(void *ptr ATTRIBUTE_UNUSED, + void *ptrptr)
What if you don't add another argument, but just change the void *ptrptr to void **ptrptr. Compiler shouldn't be mad about not knowing the size resulting of de-referencing ptrptr, you get the check you want and keep the macro without side-effects. Martin
On 15.07.2014 15:27, Martin Kletzander wrote:
On Tue, Jul 15, 2014 at 02:38:36PM +0200, Michal Privoznik wrote:
Up to now it's possible to do something like this:
const char *ptr;
ptr = strdup("my example string");
VIR_FREE(ptr);
The problem is, const char * pointers should not be modified (and freeing them is kind of modification). We should avoid this. A little trick is used: assigning a const pointer into 'void *' triggers compiler warning about discarding 'const' qualifier from pointer. So the virFree() function gains new dummy argument, that is not touched anyhow, just fulfills the const correctness check duty.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viralloc.c | 6 ++++-- src/util/viralloc.h | 20 ++++++++++++++++---- src/xenapi/xenapi_utils.c | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/util/viralloc.c b/src/util/viralloc.c index be9f0fe..0134e67 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c [...] @@ -569,13 +569,15 @@ int virAllocVar(void *ptrptr,
/** * virFree: + * @ptr: dummy pointer to check const correctness * @ptrptr: pointer to pointer for address of memory to be freed * * Release the chunk of memory in the pointer pointed to by * the 'ptrptr' variable. After release, 'ptrptr' will be * updated to point to NULL. */ -void virFree(void *ptrptr) +void virFree(void *ptr ATTRIBUTE_UNUSED, + void *ptrptr)
What if you don't add another argument, but just change the void *ptrptr to void **ptrptr. Compiler shouldn't be mad about not knowing the size resulting of de-referencing ptrptr, you get the check you want and keep the macro without side-effects.
That won't work. Consider: char *tmp; VIR_FREE(tmp); which in turn is equal to: virFree(&tmp); so the &tmp is type of 'char **' while virFree() would expect 'void **' which confuses compiler. Michal
On 07/15/2014 06:38 AM, Michal Privoznik wrote:
Up to now it's possible to do something like this:
const char *ptr;
ptr = strdup("my example string");
VIR_FREE(ptr);
The problem is, const char * pointers should not be modified (and freeing them is kind of modification). We should avoid this. A little trick is used: assigning a const pointer into 'void *' triggers compiler warning about discarding 'const' qualifier from pointer. So the virFree() function gains new dummy argument, that is not touched anyhow, just fulfills the const correctness check duty.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viralloc.c | 6 ++++-- src/util/viralloc.h | 20 ++++++++++++++++---- src/xenapi/xenapi_utils.c | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-)
But if you take my suggestion in 2/4 about merely removing the 'cast-away-const' while still keeping type safety, then a single-argument virFree() should still be noisy on attempts to VIR_FREE a const pointer.
@@ -543,11 +543,23 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * @ptr: pointer holding address to be freed * * Free the memory stored in 'ptr' and update to point - * to NULL. + * to NULL. Moreover, this macro has a side effect in + * form of evaluating passed argument multiple times.
NACK. I think it is possible to use sizeof() to come up with a construct that will only do side effects once, rather than having to weaken the guarantee of VIR_FREE. Please give me some time to propose an alternative. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/15/2014 07:46 AM, Eric Blake wrote:
But if you take my suggestion in 2/4 about merely removing the 'cast-away-const' while still keeping type safety, then a single-argument virFree() should still be noisy on attempts to VIR_FREE a const pointer.
@@ -543,11 +543,23 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * @ptr: pointer holding address to be freed * * Free the memory stored in 'ptr' and update to point - * to NULL. + * to NULL. Moreover, this macro has a side effect in + * form of evaluating passed argument multiple times.
NACK. I think it is possible to use sizeof() to come up with a construct that will only do side effects once, rather than having to weaken the guarantee of VIR_FREE. Please give me some time to propose an alternative.
I didn't even need to resort to sizeof(). Here's v2, which replaces 2, 3, and 4 of this series: https://www.redhat.com/archives/libvir-list/2014-July/msg00760.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (5)
-
Eric Blake -
John Ferlan -
Ján Tomko -
Martin Kletzander -
Michal Privoznik