[v1] Bhyve driver improvements

[PATCH 01/16] Fix build errors on FreeBSD

Posted by Ryan Moeller 4 years, 9 months ago

Don't free the file string until after it has been used to print the
error message.

I was seeing `undefined symbol: rpl_ioctl` so followed the example set
by 05a38d4c4ac035ca5dac0f00ce641a8f9b087831

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
---
 src/bhyve/bhyve_process.c | 1 -
 src/conf/virnetworkobj.c  | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 519604f898..14f718403d 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -27,7 +27,6 @@
 #include <sys/types.h>
 #include <sys/sysctl.h>
 #include <sys/user.h>
-#include <sys/ioctl.h>
 #include <net/if.h>
 #include <net/if_tap.h>
 
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index 299cdba52d..1ec1585d1c 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -1894,13 +1894,12 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net,
         file = g_strdup_printf("%s/%s.xml", dir, de->d_name);
 
         portdef = virNetworkPortDefParseFile(file);
-        VIR_FREE(file);
-        file = NULL;
-
         if (!portdef) {
             VIR_WARN("Cannot parse port %s", file);
+            VIR_FREE(file);
             continue;
         }
+        VIR_FREE(file);
 
         virUUIDFormat(portdef->uuid, uuidstr);
         if (virHashAddEntry(net->ports, uuidstr, portdef) < 0)
-- 
2.24.1

Re: [PATCH 01/16] Fix build errors on FreeBSD

Posted by Ján Tomko 4 years, 9 months ago

On Sat, Feb 22, 2020 at 01:38:40AM -0500, Ryan Moeller wrote:
>Don't free the file string until after it has been used to print the
>error message.
>

These are two independent issues that should be in two separate commits.
Also, is the premature free really a build time error?

>I was seeing `undefined symbol: rpl_ioctl` so followed the example set
>by 05a38d4c4ac035ca5dac0f00ce641a8f9b087831
>

I'm confused as to why that error can be fixed by removing an include.
If I recall correctly, we used to get that kind of error when using
gnulib's includes but not linking with gnulib.

Was that on a clean build from a fresh checkout, or do you have some
leftover gnulib artefacts present?

>Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
>---
> src/bhyve/bhyve_process.c | 1 -
> src/conf/virnetworkobj.c  | 5 ++---
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
>index 519604f898..14f718403d 100644
>--- a/src/bhyve/bhyve_process.c
>+++ b/src/bhyve/bhyve_process.c
>@@ -27,7 +27,6 @@
> #include <sys/types.h>
> #include <sys/sysctl.h>
> #include <sys/user.h>
>-#include <sys/ioctl.h>

At any rate, this header does not seem to be needed since
commit a1bd8d2546c3e469f6a5ce119fad7da1cd473db5 and can be removed,
I'm just confused about the commit message.

> #include <net/if.h>
> #include <net/if_tap.h>
>
>diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
>index 299cdba52d..1ec1585d1c 100644
>--- a/src/conf/virnetworkobj.c
>+++ b/src/conf/virnetworkobj.c
>@@ -1894,13 +1894,12 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net,
>         file = g_strdup_printf("%s/%s.xml", dir, de->d_name);
>
>         portdef = virNetworkPortDefParseFile(file);

Since the 'file' variable is only used once per scope, you can just
declare it as:
   g_autofree char *file = NULL;
and remove all the VIR_FREE calls.

Jano

>-        VIR_FREE(file);
>-        file = NULL;
>-
>         if (!portdef) {
>             VIR_WARN("Cannot parse port %s", file);
>+            VIR_FREE(file);
>             continue;
>         }
>+        VIR_FREE(file);
>
>         virUUIDFormat(portdef->uuid, uuidstr);
>         if (virHashAddEntry(net->ports, uuidstr, portdef) < 0)
>-- 
>2.24.1
>
>

Re: [PATCH 01/16] Fix build errors on FreeBSD

Posted by Ryan Moeller 4 years, 9 months ago

> I'm confused as to why that error can be fixed by removing an include.
> If I recall correctly, we used to get that kind of error when using
> gnulib's includes but not linking with gnulib.
>
> Was that on a clean build from a fresh checkout, or do you have some
> leftover gnulib artefacts present?

> At any rate, this header does not seem to be needed since
> commit a1bd8d2546c3e469f6a5ce119fad7da1cd473db5 and can be removed,
> I'm just confused about the commit message.

This patch set was started several months ago, maybe this is no longer relevant.

> Since the 'file' variable is only used once per scope, you can just
> declare it as:
>    g_autofree char *file = NULL;
> and remove all the VIR_FREE calls.

Thanks for the suggestion. I'm not at all familiar with the mailing list
workflow, can you instruct me how to submit an updated patch?

-- 
Ryan Moeller
iXsystems, Inc.
OS Developer
Email: ryan@iXsystems.com