The first two patches stem from me playing with split daemons. The rest fixes code in/around virRandomGenerateWWN() since that's where the second patch led me to. Michal Prívozník (5): virsh: Make cmdVersion() work with split daemon virrandom: Accept "nodedev" driver in virRandomGenerateWWN() virrandom: Fix printf format string in virRandomGenerateWWN() test_driver: Pass virt_type to virNodeDeviceDefParse() in testNodeDeviceCreateXML() virrandommock: Drop virRandomGenerateWWN src/test/test_driver.c | 7 +++++-- src/util/virrandom.c | 14 +++++++++++--- tests/virrandommock.c | 8 -------- tools/virsh-host.c | 26 ++++++++++++-------------- 4 files changed, 28 insertions(+), 27 deletions(-) -- 2.41.0
When virsh connects to a non-hypervisor daemon directly (e.g. "nodedev:///system") and user executes 'version' they are met with an error message. This is because cmdVersion() calls virConnectGetVersion() which fails, hence the error. The reason for virConnectGetVersion() fail is simple - it's documented as: Get the version level of the Hypervisor running. Well, there's no hypervisor in non-hypervisor daemons and thus it doesn't make sense to provide an implementation in each driver's virConnectDriver.hypervisorDriver table (just like we do for other APIs, e.g. nodeConnectIsSecure()). Given all of this, just make cmdVersion() deal with the error in a non-fatal fashion. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-host.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 0bda327cae..35e6a2eb98 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1447,21 +1447,19 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType, major, minor, rel); - if (virConnectGetVersion(priv->conn, &hvVersion) < 0) { - vshError(ctl, "%s", _("failed to get the hypervisor version")); - return false; - } - if (hvVersion == 0) { - vshPrint(ctl, - _("Cannot extract running %1$s hypervisor version\n"), hvType); - } else { - major = hvVersion / 1000000; - hvVersion %= 1000000; - minor = hvVersion / 1000; - rel = hvVersion % 1000; + if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) { + if (hvVersion == 0) { + vshPrint(ctl, + _("Cannot extract running %1$s hypervisor version\n"), hvType); + } else { + major = hvVersion / 1000000; + hvVersion %= 1000000; + minor = hvVersion / 1000; + rel = hvVersion % 1000; - vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"), - hvType, major, minor, rel); + vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"), + hvType, major, minor, rel); + } } if (vshCommandOptBool(cmd, "daemon")) { -- 2.41.0
On Tue, Jul 18, 2023 at 17:27:33 +0200, Michal Privoznik wrote:
When virsh connects to a non-hypervisor daemon directly (e.g. "nodedev:///system") and user executes 'version' they are met with an error message. This is because cmdVersion() calls virConnectGetVersion() which fails, hence the error.
The reason for virConnectGetVersion() fail is simple - it's documented as:
Get the version level of the Hypervisor running.
Well, there's no hypervisor in non-hypervisor daemons and thus it doesn't make sense to provide an implementation in each driver's virConnectDriver.hypervisorDriver table (just like we do for other APIs, e.g. nodeConnectIsSecure()).
Given all of this, just make cmdVersion() deal with the error in a non-fatal fashion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-host.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 0bda327cae..35e6a2eb98 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1447,21 +1447,19 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType, major, minor, rel);
- if (virConnectGetVersion(priv->conn, &hvVersion) < 0) { - vshError(ctl, "%s", _("failed to get the hypervisor version")); - return false; - } - if (hvVersion == 0) { - vshPrint(ctl, - _("Cannot extract running %1$s hypervisor version\n"), hvType); - } else { - major = hvVersion / 1000000; - hvVersion %= 1000000; - minor = hvVersion / 1000; - rel = hvVersion % 1000; + if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) { + if (hvVersion == 0) { + vshPrint(ctl, + _("Cannot extract running %1$s hypervisor version\n"), hvType); + } else { + major = hvVersion / 1000000; + hvVersion %= 1000000; + minor = hvVersion / 1000; + rel = hvVersion % 1000;
- vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"), - hvType, major, minor, rel); + vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"), + hvType, major, minor, rel); + } }
Ideally you'd also add an else branch with 'vshResetLibvirtError(); but the other call to virConnectGetLibVersion() doesn't do that so ... whatever ... I guess :)
if (vshCommandOptBool(cmd, "daemon")) { -- 2.41.0
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 7/18/23 17:47, Peter Krempa wrote:
On Tue, Jul 18, 2023 at 17:27:33 +0200, Michal Privoznik wrote:
When virsh connects to a non-hypervisor daemon directly (e.g. "nodedev:///system") and user executes 'version' they are met with an error message. This is because cmdVersion() calls virConnectGetVersion() which fails, hence the error.
The reason for virConnectGetVersion() fail is simple - it's documented as:
Get the version level of the Hypervisor running.
Well, there's no hypervisor in non-hypervisor daemons and thus it doesn't make sense to provide an implementation in each driver's virConnectDriver.hypervisorDriver table (just like we do for other APIs, e.g. nodeConnectIsSecure()).
Given all of this, just make cmdVersion() deal with the error in a non-fatal fashion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-host.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 0bda327cae..35e6a2eb98 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1447,21 +1447,19 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType, major, minor, rel);
- if (virConnectGetVersion(priv->conn, &hvVersion) < 0) { - vshError(ctl, "%s", _("failed to get the hypervisor version")); - return false; - } - if (hvVersion == 0) { - vshPrint(ctl, - _("Cannot extract running %1$s hypervisor version\n"), hvType); - } else { - major = hvVersion / 1000000; - hvVersion %= 1000000; - minor = hvVersion / 1000; - rel = hvVersion % 1000; + if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) { + if (hvVersion == 0) { + vshPrint(ctl, + _("Cannot extract running %1$s hypervisor version\n"), hvType); + } else { + major = hvVersion / 1000000; + hvVersion %= 1000000; + minor = hvVersion / 1000; + rel = hvVersion % 1000;
- vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"), - hvType, major, minor, rel); + vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"), + hvType, major, minor, rel); + } }
Ideally you'd also add an else branch with 'vshResetLibvirtError(); but the other call to virConnectGetLibVersion() doesn't do that so ... whatever ... I guess :)
Oh, you're right. In fact we should also ignore VIR_ERR_NO_SUPPORT. Other errors might be actually a good reason to return early. Consider this squashed in: diff --git i/tools/virsh-host.c w/tools/virsh-host.c index 35e6a2eb98..ad440d5123 100644 --- i/tools/virsh-host.c +++ w/tools/virsh-host.c @@ -1447,7 +1447,14 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType, major, minor, rel); - if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) { + if (virConnectGetVersion(priv->conn, &hvVersion) < 0) { + if (last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + } else { + vshError(ctl, "%s", _("failed to get the hypervisor version")); + return false; + } + } else {
if (vshCommandOptBool(cmd, "daemon")) { -- 2.41.0
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Thanks, Michal
The virRandomGenerateWWN() is used solely by nodedev driver to autogenerate WWNN and WWNP when parsing a nodedev XML. Now, the idea was (at least during monolithic daemon) that depending on which hypervisor driver called the nodedev XML parsing (and virRandomGenerateWWN() under the hood) the corresponding OUI is used (e.g. "001a4a" for the QEMU driver). But in era of split daemons things are not that easy. We do not know which hypervisor driver called us. And there might be no hypervisor driver at all - users are allowed to connect to individual drivers directly (e.g. "nodedev:///system"). In this case, we can't use proper OUI. Well, do the next best thing: pick one. By a fair roll of dice the one used by the QEMU driver (QUMRANET_OUI) was chosen. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 73c5832a05..7606dd1684 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -138,7 +138,13 @@ virRandomGenerateWWN(char **wwn, return -1; } - if (STREQ(virt_type, "QEMU")) { + /* In case of split daemon we don't really see the hypervisor + * driver that just re-routed the nodedev driver API. There + * might not be any hypervisor driver even. Yet, we have to + * pick OUI. Pick "QEMU". */ + + if (STREQ(virt_type, "QEMU") || + STREQ(virt_type, "nodedev")) { oui = QUMRANET_OUI; } else if (STREQ(virt_type, "Xen") || STREQ(virt_type, "xenlight")) { -- 2.41.0
On Tue, Jul 18, 2023 at 17:27:35 +0200, Michal Privoznik wrote:
The virRandomGenerateWWN() is used solely by nodedev driver to autogenerate WWNN and WWNP when parsing a nodedev XML. Now, the idea was (at least during monolithic daemon) that depending on which hypervisor driver called the nodedev XML parsing (and virRandomGenerateWWN() under the hood) the corresponding OUI is used (e.g. "001a4a" for the QEMU driver).
But in era of split daemons things are not that easy. We do not know which hypervisor driver called us. And there might be no hypervisor driver at all - users are allowed to connect to individual drivers directly (e.g. "nodedev:///system").
In this case, we can't use proper OUI. Well, do the next best thing: pick one. By a fair roll of dice the one used by the QEMU driver (QUMRANET_OUI) was chosen.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 73c5832a05..7606dd1684 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -138,7 +138,13 @@ virRandomGenerateWWN(char **wwn, return -1; }
- if (STREQ(virt_type, "QEMU")) { + /* In case of split daemon we don't really see the hypervisor + * driver that just re-routed the nodedev driver API. There + * might not be any hypervisor driver even. Yet, we have to + * pick OUI. Pick "QEMU". */ + + if (STREQ(virt_type, "QEMU") || + STREQ(virt_type, "nodedev")) { oui = QUMRANET_OUI; } else if (STREQ(virt_type, "Xen") || STREQ(virt_type, "xenlight")) {
I at first wanted to suggest to simply drop the code selecting the prefix based on the hypervisor type, but this would introduce a behaviour change for any existing monolithic deployment. Since this simply didn't work with modular daemons until now I guess we can argue to change the behaviour in the way you propose. Two other solution I can see in this case are: - use our own OUI (but we don't have one) - try to plumb the hypervisor name if possible through a new API (complex, unclear benefit, doesn't solve all cases anywyas) From my side, please drop the commit message bit about it being chosen by a dice roll. I don't really buy that. Additionally please allow others to chime in (don't push it right after): Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Firstly, drop needless concatenation of two static strings. Secondly, use proper (portable) formatter for uint64_t so that typecast to ULL can be dropped. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 7606dd1684..38fcfbc6ba 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -160,8 +160,7 @@ virRandomGenerateWWN(char **wwn, return -1; } - *wwn = g_strdup_printf("5" "%s%09llx", oui, - (unsigned long long)virRandomBits(36)); + *wwn = g_strdup_printf("5%s%09" PRIx64, oui, virRandomBits(36)); return 0; } -- 2.41.0
On Tue, Jul 18, 2023 at 17:27:36 +0200, Michal Privoznik wrote:
Firstly, drop needless concatenation of two static strings. Secondly, use proper (portable) formatter for uint64_t so that typecast to ULL can be dropped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This brings the code closer to real implementation: nodeDeviceCreateXML(). For the unique OUI, let's take the value from tests/virrandommock.c: 100000. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 7 +++++-- src/util/virrandom.c | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e7fce053b4..3767908d9d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7685,11 +7685,14 @@ testNodeDeviceCreateXML(virConnectPtr conn, g_autofree char *wwnn = NULL; g_autofree char *wwpn = NULL; bool validate = flags & VIR_NODE_DEVICE_CREATE_XML_VALIDATE; + const char *virt_type; virCheckFlags(VIR_NODE_DEVICE_CREATE_XML_VALIDATE, NULL); - if (!(def = virNodeDeviceDefParse(xmlDesc, NULL, CREATE_DEVICE, NULL, NULL, - NULL, validate))) + virt_type = virConnectGetType(conn); + + if (!(def = virNodeDeviceDefParse(xmlDesc, NULL, CREATE_DEVICE, virt_type, + NULL, NULL, validate))) goto cleanup; /* We run this simply for validation - it essentially validates that diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 38fcfbc6ba..11f3a94611 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -124,6 +124,7 @@ virRandomBytes(unsigned char *buf, #define VMWARE_OUI "000569" #define MICROSOFT_OUI "0050f2" #define XEN_OUI "00163e" +#define TEST_DRIVER_OUI "100000" int @@ -154,6 +155,8 @@ virRandomGenerateWWN(char **wwn, oui = VMWARE_OUI; } else if (STREQ(virt_type, "HYPER-V")) { oui = MICROSOFT_OUI; + } else if (STREQ(virt_type, "TEST")) { + oui = TEST_DRIVER_OUI; } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported virt type")); -- 2.41.0
On Tue, Jul 18, 2023 at 17:27:37 +0200, Michal Privoznik wrote:
This brings the code closer to real implementation: nodeDeviceCreateXML(). For the unique OUI, let's take the value from tests/virrandommock.c: 100000.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 7 +++++-- src/util/virrandom.c | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
After previous commit, there's no functional difference between real virRandomGenerateWWN() and the mocked version. Drop the mock then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virrandommock.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/virrandommock.c b/tests/virrandommock.c index 2673230cf7..17c55f4c60 100644 --- a/tests/virrandommock.c +++ b/tests/virrandommock.c @@ -47,14 +47,6 @@ uint64_t virRandomBits(int nbits) return ret; } -int virRandomGenerateWWN(char **wwn, - const char *virt_type G_GNUC_UNUSED) -{ - *wwn = g_strdup_printf("5100000%09llx", - (unsigned long long)virRandomBits(36)); - return 0; -} - #else /* WIN32 */ /* Can't mock on WIN32 */ #endif -- 2.41.0
On Tue, Jul 18, 2023 at 17:27:38 +0200, Michal Privoznik wrote:
After previous commit, there's no functional difference between real virRandomGenerateWWN() and the mocked version. Drop the mock then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virrandommock.c | 8 -------- 1 file changed, 8 deletions(-)
Also adjust the following line, as this function isn't being mocked any more: src/util/virrandom.h:int virRandomGenerateWWN(char **wwn, const char *virt_type) G_NO_INLINE; Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa