[libvirt] [PATCH 0/3] Remove unused macros
Ján Tomko (3): Remove unused SOL_NETLINK macro vbox: remove duplicate macros vsh: remove namespace poisoning src/util/virnetlink.c | 4 ---- src/vbox/vbox_driver.c | 2 -- src/vbox/vbox_network.c | 43 ------------------------------------------- tools/vsh.c | 6 ------ tools/vsh.h | 10 ---------- 5 files changed, 65 deletions(-) -- 2.7.3
Introduced by commit d575679, unused at the time. --- src/util/virnetlink.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 5491ece..513f36e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -42,10 +42,6 @@ #include "virmacaddr.h" #include "virerror.h" -#ifndef SOL_NETLINK -# define SOL_NETLINK 270 -#endif - #define VIR_FROM_THIS VIR_FROM_NET VIR_LOG_INIT("util.netlink"); -- 2.7.3
On 06/20/2016 11:28 AM, Ján Tomko wrote:
Introduced by commit d575679, unused at the time. --- src/util/virnetlink.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 5491ece..513f36e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -42,10 +42,6 @@ #include "virmacaddr.h" #include "virerror.h"
-#ifndef SOL_NETLINK -# define SOL_NETLINK 270 -#endif - #define VIR_FROM_THIS VIR_FROM_NET
VIR_LOG_INIT("util.netlink");
Provisional ACK since you are correct that it isn't visibly used, but I can't help thinking the author had some reason for adding it. It looks like SOL_NETLINK wasn't present in kernel 2.6 (RHEL6 vintage - although it is mentioned in a comment of one of the system #includes), but was there by the time of kernel 3.10 (RHEL7). I Cc'ed the author's email on this reply. If we don't hear back from him, I guess we can assume it was something put in to test an alternate netlink protocol, and we can add it back in later if it causes a build problem on some platform, or is needed for something in the future. (I did try a stock build on RHEL6 with this chunk removed, and it had no problems; that, combined with the fact that it's defined below all the #includes in the file shows that it's not a case of a reference hidden in an include file somewhere).
On Mon, Jun 20, 2016 at 01:43:30PM -0400, Laine Stump wrote:
On 06/20/2016 11:28 AM, Ján Tomko wrote:
Introduced by commit d575679, unused at the time. --- src/util/virnetlink.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 5491ece..513f36e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -42,10 +42,6 @@ #include "virmacaddr.h" #include "virerror.h"
-#ifndef SOL_NETLINK -# define SOL_NETLINK 270 -#endif - #define VIR_FROM_THIS VIR_FROM_NET
VIR_LOG_INIT("util.netlink");
Provisional ACK since you are correct that it isn't visibly used, but I can't help thinking the author had some reason for adding it.
An earlier version of the patchset used it to call: setsockopt(fd, SOL_NETLINK, NETLINK_ADD_MEMBERSHIP, https://www.redhat.com/archives/libvir-list/2012-August/msg01457.html In a later version it was changed to nl_socket_add_membership without removing the macro (since we disable -Wunused-macros by default): https://www.redhat.com/archives/libvir-list/2012-August/msg01541.html Sadly, this function is also present in libnl 1.1, which is our minimum required version, so we can't drop the #ifdev HAVE_LIBNL1 code yet.
It looks like SOL_NETLINK wasn't present in kernel 2.6 (RHEL6 vintage - although it is mentioned in a comment of one of the system #includes), but was there by the time of kernel 3.10 (RHEL7).
It was introduced by commit 9a4595bc7e released in kernel v2.6.14, so RHEL6 based on 2.6.32 had it from the start. Jan
On 06/21/2016 04:17 AM, Ján Tomko wrote:
On Mon, Jun 20, 2016 at 01:43:30PM -0400, Laine Stump wrote:
On 06/20/2016 11:28 AM, Ján Tomko wrote:
Introduced by commit d575679, unused at the time. --- src/util/virnetlink.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 5491ece..513f36e 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -42,10 +42,6 @@ #include "virmacaddr.h" #include "virerror.h"
-#ifndef SOL_NETLINK -# define SOL_NETLINK 270 -#endif - #define VIR_FROM_THIS VIR_FROM_NET
VIR_LOG_INIT("util.netlink");
Provisional ACK since you are correct that it isn't visibly used, but I can't help thinking the author had some reason for adding it.
An earlier version of the patchset used it to call: setsockopt(fd, SOL_NETLINK, NETLINK_ADD_MEMBERSHIP, https://www.redhat.com/archives/libvir-list/2012-August/msg01457.html
In a later version it was changed to nl_socket_add_membership without removing the macro (since we disable -Wunused-macros by default): https://www.redhat.com/archives/libvir-list/2012-August/msg01541.html
Okay, so that explains how it got there, and proves that it's unneeded, so the provisional ACK is now full :-)
Sadly, this function is also present in libnl 1.1, which is our minimum required version, so we can't drop the #ifdev HAVE_LIBNL1 code yet.
I don't understand the relationship. LIBNL1 is used in RHEL6 because for a very long time libnl3 wasn't available there. At some point relatively far into the life of RHEL6 the libnl-3 package was added (but appears to only be used by libiwpm and sssd-common; I had *thought* that NetworkManager in RHEL6 had been the reason for the addition, but I guess I remembered wrong). At any rate it's probably not a good idea to switch even upstream RHEL6/CentOS6 builds (which are likely the only reason left to keep libnl1) over to the RHEL6 libnl-3 library, because it was added so later, is used almost nowhere else, and is such an old version. So unfortunately we will have to keep the libnl1 support around for a long while yet.
It looks like SOL_NETLINK wasn't present in kernel 2.6 (RHEL6 vintage - although it is mentioned in a comment of one of the system #includes), but was there by the time of kernel 3.10 (RHEL7).
It was introduced by commit 9a4595bc7e released in kernel v2.6.14, so RHEL6 based on 2.6.32 had it from the start.
It may seem that way, but actually isn't. The problem is that whatever file it was originally put in didn't get into the kernel-headers package. So "find /etc -name \*.h | xargs grep SOL_NETLINK" finds nothing, and if you try to use SOL_NETLINK in a RHEL6 build of libvirt with these extra lines removed, it will fail. (Yes, I tried it) (That's not a reason to not rip out these lines, just illustrating that all is not always as it seems :-)
There is a definiton of VIR_FROM_THIS just two lines above. The rest is defined in vbox_common.h. --- src/vbox/vbox_driver.c | 2 -- src/vbox/vbox_network.c | 43 ------------------------------------------- 2 files changed, 45 deletions(-) diff --git a/src/vbox/vbox_driver.c b/src/vbox/vbox_driver.c index a5f3721..b3b4ee6 100644 --- a/src/vbox/vbox_driver.c +++ b/src/vbox/vbox_driver.c @@ -48,8 +48,6 @@ VIR_LOG_INIT("vbox.vbox_driver"); -#define VIR_FROM_THIS VIR_FROM_VBOX - #if !defined(WITH_DRIVER_MODULES) || defined(VBOX_DRIVER) static virDrvOpenStatus dummyConnectOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, diff --git a/src/vbox/vbox_network.c b/src/vbox/vbox_network.c index ee409e3..4cfc365 100644 --- a/src/vbox/vbox_network.c +++ b/src/vbox/vbox_network.c @@ -37,49 +37,6 @@ VIR_LOG_INIT("vbox.vbox_network"); -#define RC_SUCCEEDED(rc) NS_SUCCEEDED(rc.resultCode) -#define RC_FAILED(rc) NS_FAILED(rc.resultCode) - -#define VBOX_UTF16_FREE(arg) \ - do { \ - if (arg) { \ - gVBoxAPI.UPFN.Utf16Free(data->pFuncs, arg); \ - (arg) = NULL; \ - } \ - } while (0) - -#define VBOX_UTF8_FREE(arg) \ - do { \ - if (arg) { \ - gVBoxAPI.UPFN.Utf8Free(data->pFuncs, arg); \ - (arg) = NULL; \ - } \ - } while (0) - -#define VBOX_UTF16_TO_UTF8(arg1, arg2) gVBoxAPI.UPFN.Utf16ToUtf8(data->pFuncs, arg1, arg2) -#define VBOX_UTF8_TO_UTF16(arg1, arg2) gVBoxAPI.UPFN.Utf8ToUtf16(data->pFuncs, arg1, arg2) - -#define VBOX_RELEASE(arg) \ - do { \ - if (arg) { \ - gVBoxAPI.nsUISupports.Release((void *)arg); \ - (arg) = NULL; \ - } \ - } while (0) - -#define vboxIIDUnalloc(iid) gVBoxAPI.UIID.vboxIIDUnalloc(data, iid) -#define vboxIIDToUUID(iid, uuid) gVBoxAPI.UIID.vboxIIDToUUID(data, iid, uuid) -#define vboxIIDFromUUID(iid, uuid) gVBoxAPI.UIID.vboxIIDFromUUID(data, iid, uuid) -#define vboxIIDIsEqual(iid1, iid2) gVBoxAPI.UIID.vboxIIDIsEqual(data, iid1, iid2) -#define DEBUGIID(msg, iid) gVBoxAPI.UIID.DEBUGIID(msg, iid) -#define vboxIIDFromArrayItem(iid, array, idx) \ - gVBoxAPI.UIID.vboxIIDFromArrayItem(data, iid, array, idx) - -#define VBOX_IID_INITIALIZE(iid) gVBoxAPI.UIID.vboxIIDInitialize(iid) - -#define ARRAY_GET_MACHINES \ - (gVBoxAPI.UArray.handleGetMachines(data->vboxObj)) - static vboxUniformedAPI gVBoxAPI; /** -- 2.7.3
On 06/20/2016 11:28 AM, Ján Tomko wrote:
There is a definiton of VIR_FROM_THIS just two lines above.
The rest is defined in vbox_common.h. --- src/vbox/vbox_driver.c | 2 -- src/vbox/vbox_network.c | 43 ------------------------------------------- 2 files changed, 45 deletions(-)
ACK.
We already have a syntax-check to prohibit direct use of these allocation functions. --- tools/vsh.c | 6 ------ tools/vsh.h | 10 ---------- 2 files changed, 16 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 8649305..2b78919 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -71,9 +71,6 @@ const vshCmdGrp *cmdGroups; const vshCmdDef *cmdSet; -/* Bypass header poison */ -#undef strdup - /* simple handler for oom conditions */ static void @@ -164,9 +161,6 @@ _vshStrdup(vshControl *ctl, const char *s, const char *filename, int line) exit(EXIT_FAILURE); } -/* Poison the raw allocating identifiers in favor of our vsh variants. */ -#define strdup use_vshStrdup_instead_of_strdup - int vshNameSorter(const void *a, const void *b) { diff --git a/tools/vsh.h b/tools/vsh.h index f738a6f..8d67397 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -453,16 +453,6 @@ char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, int line); # define vshStrdup(_ctl, _s) _vshStrdup(_ctl, _s, __FILE__, __LINE__) -/* Poison the raw allocating identifiers in favor of our vsh variants. */ -# undef malloc -# undef calloc -# undef realloc -# undef strdup -# define malloc use_vshMalloc_instead_of_malloc -# define calloc use_vshCalloc_instead_of_calloc -# define realloc use_vshRealloc_instead_of_realloc -# define strdup use_vshStrdup_instead_of_strdup - /* Macros to help dealing with mutually exclusive options. */ /* VSH_EXCLUSIVE_OPTIONS_EXPR: -- 2.7.3
On 06/20/2016 11:28 AM, Ján Tomko wrote:
We already have a syntax-check to prohibit direct use of these allocation functions.
Yep. This was added in commit d9adac in 2010, well before we had such syntax rules, but it's pointless now. ACK.
--- tools/vsh.c | 6 ------ tools/vsh.h | 10 ---------- 2 files changed, 16 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 8649305..2b78919 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -71,9 +71,6 @@ const vshCmdGrp *cmdGroups; const vshCmdDef *cmdSet;
-/* Bypass header poison */ -#undef strdup -
/* simple handler for oom conditions */ static void @@ -164,9 +161,6 @@ _vshStrdup(vshControl *ctl, const char *s, const char *filename, int line) exit(EXIT_FAILURE); }
-/* Poison the raw allocating identifiers in favor of our vsh variants. */ -#define strdup use_vshStrdup_instead_of_strdup - int vshNameSorter(const void *a, const void *b) { diff --git a/tools/vsh.h b/tools/vsh.h index f738a6f..8d67397 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -453,16 +453,6 @@ char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, int line); # define vshStrdup(_ctl, _s) _vshStrdup(_ctl, _s, __FILE__, __LINE__)
-/* Poison the raw allocating identifiers in favor of our vsh variants. */ -# undef malloc -# undef calloc -# undef realloc -# undef strdup -# define malloc use_vshMalloc_instead_of_malloc -# define calloc use_vshCalloc_instead_of_calloc -# define realloc use_vshRealloc_instead_of_realloc -# define strdup use_vshStrdup_instead_of_strdup - /* Macros to help dealing with mutually exclusive options. */
/* VSH_EXCLUSIVE_OPTIONS_EXPR:
participants (2)
-
Ján Tomko -
Laine Stump