Provide minimal support for hotplugging ETHERNET or BRIDGE type NICs in
the test driver.
Signed-off-by: John Levon <john.levon@nutanix.com>
---
src/test/test_driver.c | 146 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 137 insertions(+), 9 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 7cb77f044d..4915c13a25 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -38,6 +38,7 @@
#include "virnetworkobj.h"
#include "interface_conf.h"
#include "checkpoint_conf.h"
+#include "domain_audit.h"
#include "domain_conf.h"
#include "domain_driver.h"
#include "domain_event.h"
@@ -10115,6 +10116,93 @@ testDomainAttachHostPCIDevice(testDriver *driver G_GNUC_UNUSED,
}
+static int
+testDomainDeviceAliasIndex(const virDomainDeviceInfo *info,
+ const char *prefix)
+{
+ int idx;
+
+ if (!info->alias)
+ return -1;
+ if (!STRPREFIX(info->alias, prefix))
+ return -1;
+
+ if (virStrToLong_i(info->alias + strlen(prefix), NULL, 10, &idx) < 0)
+ return -1;
+
+ return idx;
+}
+
+
+static void
+testAssignDeviceNetAlias(virDomainDef *def,
+ virDomainNetDef *net,
+ int idx)
+{
+ if (net->info.alias)
+ return;
+
+ if (idx == -1) {
+ size_t i;
+
+ idx = 0;
+ for (i = 0; i < def->nnets; i++) {
+ int thisidx;
+
+ if ((thisidx = testDomainDeviceAliasIndex(&def->nets[i]->info, "net")) < 0)
+ continue; /* failure could be due to "hostdevN" */
+ if (thisidx >= idx)
+ idx = thisidx + 1;
+ }
+ }
+
+ net->info.alias = g_strdup_printf("net%d", idx);
+}
+
+
+static int
+testDomainAttachNetDevice(testDriver *driver G_GNUC_UNUSED,
+ virDomainObj *vm,
+ virDomainNetDef *net)
+{
+ virDomainNetType actualType;
+
+ actualType = virDomainNetGetActualType(net);
+
+ testAssignDeviceNetAlias(vm->def, net, -1);
+
+ switch (actualType) {
+ case VIR_DOMAIN_NET_TYPE_ETHERNET:
+ case VIR_DOMAIN_NET_TYPE_BRIDGE:
+ break;
+
+ case VIR_DOMAIN_NET_TYPE_USER:
+ case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
+ case VIR_DOMAIN_NET_TYPE_SERVER:
+ case VIR_DOMAIN_NET_TYPE_CLIENT:
+ case VIR_DOMAIN_NET_TYPE_MCAST:
+ case VIR_DOMAIN_NET_TYPE_NETWORK:
+ case VIR_DOMAIN_NET_TYPE_INTERNAL:
+ case VIR_DOMAIN_NET_TYPE_DIRECT:
+ case VIR_DOMAIN_NET_TYPE_HOSTDEV:
+ case VIR_DOMAIN_NET_TYPE_UDP:
+ case VIR_DOMAIN_NET_TYPE_VDPA:
+ case VIR_DOMAIN_NET_TYPE_NULL:
+ case VIR_DOMAIN_NET_TYPE_VDS:
+ case VIR_DOMAIN_NET_TYPE_LAST:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("hotplug of interface type of %1$s is not implemented yet"),
+ virDomainNetTypeToString(actualType));
+ return -1;
+ }
+
+ VIR_APPEND_ELEMENT_COPY(vm->def->nets, vm->def->nnets, net);
+
+ virDomainAuditNet(vm, NULL, net, "attach", true);
+
+ return 0;
+}
+
static int
testDomainAttachHostDevice(testDriver *driver,
virDomainObj *vm,
@@ -10144,28 +10232,68 @@ testDomainAttachDeviceLive(virDomainObj *vm,
testDriver *driver)
{
const char *alias = NULL;
+ int ret = -1;
- if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_NET:
+ testDomainObjCheckNetTaint(vm, dev->data.net);
+ ret = testDomainAttachNetDevice(driver, vm, dev->data.net);
+ if (!ret) {
+ alias = dev->data.net->info.alias;
+ dev->data.net = NULL;
+ }
+ break;
+
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
+ ret = testDomainAttachHostDevice(driver, vm,
+ dev->data.hostdev);
+ if (!ret) {
+ alias = dev->data.hostdev->info->alias;
+ dev->data.hostdev = NULL;
+ }
+ break;
+
+ case VIR_DOMAIN_DEVICE_NONE:
+ case VIR_DOMAIN_DEVICE_DISK:
+ case VIR_DOMAIN_DEVICE_LEASE:
+ case VIR_DOMAIN_DEVICE_FS:
+ case VIR_DOMAIN_DEVICE_INPUT:
+ case VIR_DOMAIN_DEVICE_SOUND:
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ case VIR_DOMAIN_DEVICE_HUB:
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ case VIR_DOMAIN_DEVICE_SMARTCARD:
+ case VIR_DOMAIN_DEVICE_CHR:
+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
+ case VIR_DOMAIN_DEVICE_NVRAM:
+ case VIR_DOMAIN_DEVICE_RNG:
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ case VIR_DOMAIN_DEVICE_TPM:
+ case VIR_DOMAIN_DEVICE_PANIC:
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ case VIR_DOMAIN_DEVICE_IOMMU:
+ case VIR_DOMAIN_DEVICE_VSOCK:
+ case VIR_DOMAIN_DEVICE_AUDIO:
+ case VIR_DOMAIN_DEVICE_CRYPTO:
+ case VIR_DOMAIN_DEVICE_PSTORE:
+ case VIR_DOMAIN_DEVICE_LAST:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("live attach of device '%1$s' is not supported"),
virDomainDeviceTypeToString(dev->type));
return -1;
}
- testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
- if (testDomainAttachHostDevice(driver, vm, dev->data.hostdev) < 0)
- return -1;
-
- alias = dev->data.hostdev->info->alias;
- dev->data.hostdev = NULL;
-
if (alias) {
virObjectEvent *event;
event = virDomainEventDeviceAddedNewFromObj(vm, alias);
virObjectEventStateQueue(driver->eventState, event);
}
- return 0;
+ return ret;
}
--
2.34.1On Thu, Aug 01, 2024 at 12:47:40PM +0100, John Levon wrote:
>Provide minimal support for hotplugging ETHERNET or BRIDGE type NICs in
>the test driver.
>
>Signed-off-by: John Levon <john.levon@nutanix.com>
>---
> src/test/test_driver.c | 146 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 137 insertions(+), 9 deletions(-)
>
>diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>index 7cb77f044d..4915c13a25 100644
>--- a/src/test/test_driver.c
>+++ b/src/test/test_driver.c
>@@ -38,6 +38,7 @@
> #include "virnetworkobj.h"
> #include "interface_conf.h"
> #include "checkpoint_conf.h"
>+#include "domain_audit.h"
> #include "domain_conf.h"
> #include "domain_driver.h"
> #include "domain_event.h"
>@@ -10115,6 +10116,93 @@ testDomainAttachHostPCIDevice(testDriver *driver G_GNUC_UNUSED,
> }
>
>
>+static int
>+testDomainDeviceAliasIndex(const virDomainDeviceInfo *info,
>+ const char *prefix)
>+{
>+ int idx;
>+
>+ if (!info->alias)
>+ return -1;
>+ if (!STRPREFIX(info->alias, prefix))
>+ return -1;
>+
>+ if (virStrToLong_i(info->alias + strlen(prefix), NULL, 10, &idx) < 0)
>+ return -1;
>+
>+ return idx;
>+}
>+
>+
>+static void
>+testAssignDeviceNetAlias(virDomainDef *def,
>+ virDomainNetDef *net,
>+ int idx)
>+{
>+ if (net->info.alias)
>+ return;
>+
>+ if (idx == -1) {
>+ size_t i;
>+
>+ idx = 0;
>+ for (i = 0; i < def->nnets; i++) {
>+ int thisidx;
>+
>+ if ((thisidx = testDomainDeviceAliasIndex(&def->nets[i]->info, "net")) < 0)
>+ continue; /* failure could be due to "hostdevN" */
>+ if (thisidx >= idx)
>+ idx = thisidx + 1;
>+ }
>+ }
>+
>+ net->info.alias = g_strdup_printf("net%d", idx);
>+}
>+
>+
>+static int
>+testDomainAttachNetDevice(testDriver *driver G_GNUC_UNUSED,
>+ virDomainObj *vm,
>+ virDomainNetDef *net)
>+{
>+ virDomainNetType actualType;
>+
>+ actualType = virDomainNetGetActualType(net);
>+
>+ testAssignDeviceNetAlias(vm->def, net, -1);
>+
>+ switch (actualType) {
>+ case VIR_DOMAIN_NET_TYPE_ETHERNET:
>+ case VIR_DOMAIN_NET_TYPE_BRIDGE:
>+ break;
>+
>+ case VIR_DOMAIN_NET_TYPE_USER:
>+ case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>+ case VIR_DOMAIN_NET_TYPE_SERVER:
>+ case VIR_DOMAIN_NET_TYPE_CLIENT:
>+ case VIR_DOMAIN_NET_TYPE_MCAST:
>+ case VIR_DOMAIN_NET_TYPE_NETWORK:
>+ case VIR_DOMAIN_NET_TYPE_INTERNAL:
>+ case VIR_DOMAIN_NET_TYPE_DIRECT:
>+ case VIR_DOMAIN_NET_TYPE_HOSTDEV:
>+ case VIR_DOMAIN_NET_TYPE_UDP:
>+ case VIR_DOMAIN_NET_TYPE_VDPA:
>+ case VIR_DOMAIN_NET_TYPE_NULL:
>+ case VIR_DOMAIN_NET_TYPE_VDS:
>+ case VIR_DOMAIN_NET_TYPE_LAST:
>+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>+ _("hotplug of interface type of %1$s is not implemented yet"),
>+ virDomainNetTypeToString(actualType));
>+ return -1;
>+ }
>+
>+ VIR_APPEND_ELEMENT_COPY(vm->def->nets, vm->def->nnets, net);
>+
>+ virDomainAuditNet(vm, NULL, net, "attach", true);
>+
No need for an audit in test driver I think.
I wanted to say that there are no additional checks for the device being
done, but that corresponds with starting the domain as well, so that's fine.
>+ return 0;
>+}
>+
> static int
> testDomainAttachHostDevice(testDriver *driver,
> virDomainObj *vm,
>@@ -10144,28 +10232,68 @@ testDomainAttachDeviceLive(virDomainObj *vm,
> testDriver *driver)
> {
> const char *alias = NULL;
>+ int ret = -1;
>
>- if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
>+ switch (dev->type) {
>+ case VIR_DOMAIN_DEVICE_NET:
>+ testDomainObjCheckNetTaint(vm, dev->data.net);
>+ ret = testDomainAttachNetDevice(driver, vm, dev->data.net);
>+ if (!ret) {
if this is an integer where 0 is success and negative number is an error
we tend to check for (ret == 0). In this case it does not do much, but
in other instances it allows us to add different success return values
in the future. And keeping the same way of doing things makes it nicer
as consistent appearance is easier to read.
Since you are not using @ret anywhere else you can just do if (func() ==
0) or even better if (func() < 0) return -1; and then return 0 at the
end in case there is no error.
>+ alias = dev->data.net->info.alias;
>+ dev->data.net = NULL;
>+ }
>+ break;
>+
>+ case VIR_DOMAIN_DEVICE_HOSTDEV:
>+ testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
>+ ret = testDomainAttachHostDevice(driver, vm,
>+ dev->data.hostdev);
>+ if (!ret) {
same here
>+ alias = dev->data.hostdev->info->alias;
>+ dev->data.hostdev = NULL;
And since these two lines are the same for both code paths you can do
that after the switch when you error out early ...
>+ }
>+ break;
>+
>+ case VIR_DOMAIN_DEVICE_NONE:
>+ case VIR_DOMAIN_DEVICE_DISK:
>+ case VIR_DOMAIN_DEVICE_LEASE:
>+ case VIR_DOMAIN_DEVICE_FS:
>+ case VIR_DOMAIN_DEVICE_INPUT:
>+ case VIR_DOMAIN_DEVICE_SOUND:
>+ case VIR_DOMAIN_DEVICE_VIDEO:
>+ case VIR_DOMAIN_DEVICE_WATCHDOG:
>+ case VIR_DOMAIN_DEVICE_CONTROLLER:
>+ case VIR_DOMAIN_DEVICE_GRAPHICS:
>+ case VIR_DOMAIN_DEVICE_HUB:
>+ case VIR_DOMAIN_DEVICE_REDIRDEV:
>+ case VIR_DOMAIN_DEVICE_SMARTCARD:
>+ case VIR_DOMAIN_DEVICE_CHR:
>+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
>+ case VIR_DOMAIN_DEVICE_NVRAM:
>+ case VIR_DOMAIN_DEVICE_RNG:
>+ case VIR_DOMAIN_DEVICE_SHMEM:
>+ case VIR_DOMAIN_DEVICE_TPM:
>+ case VIR_DOMAIN_DEVICE_PANIC:
>+ case VIR_DOMAIN_DEVICE_MEMORY:
>+ case VIR_DOMAIN_DEVICE_IOMMU:
>+ case VIR_DOMAIN_DEVICE_VSOCK:
>+ case VIR_DOMAIN_DEVICE_AUDIO:
>+ case VIR_DOMAIN_DEVICE_CRYPTO:
>+ case VIR_DOMAIN_DEVICE_PSTORE:
>+ case VIR_DOMAIN_DEVICE_LAST:
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("live attach of device '%1$s' is not supported"),
> virDomainDeviceTypeToString(dev->type));
> return -1;
> }
>
>- testDomainObjCheckHostdevTaint(vm, dev->data.hostdev);
>- if (testDomainAttachHostDevice(driver, vm, dev->data.hostdev) < 0)
>- return -1;
>-
>- alias = dev->data.hostdev->info->alias;
>- dev->data.hostdev = NULL;
>-
... which means you can keep these two lines here.
> if (alias) {
> virObjectEvent *event;
> event = virDomainEventDeviceAddedNewFromObj(vm, alias);
> virObjectEventStateQueue(driver->eventState, event);
> }
>
>- return 0;
>+ return ret;
> }
>
>
>--
>2.34.1
>
On Wed, Oct 02, 2024 at 01:00:33PM +0200, Martin Kletzander wrote:
> > static int
> > testDomainAttachHostDevice(testDriver *driver,
> > virDomainObj *vm,
> > @@ -10144,28 +10232,68 @@ testDomainAttachDeviceLive(virDomainObj *vm,
> > testDriver *driver)
> > {
> > const char *alias = NULL;
> > + int ret = -1;
> >
> > - if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
> > + switch (dev->type) {
> > + case VIR_DOMAIN_DEVICE_NET:
> > + testDomainObjCheckNetTaint(vm, dev->data.net);
> > + ret = testDomainAttachNetDevice(driver, vm, dev->data.net);
> > + if (!ret) {
>
> if this is an integer where 0 is success and negative number is an error
> we tend to check for (ret == 0). In this case it does not do much, but
> in other instances it allows us to add different success return values
> in the future. And keeping the same way of doing things makes it nicer
> as consistent appearance is easier to read.
Isn't it better to be consistent with the original code? This came directly from
qemu_hotplug.c
> > + alias = dev->data.hostdev->info->alias;
> > + dev->data.hostdev = NULL;
>
> And since these two lines are the same for both code paths you can do
> that after the switch when you error out early ...
ditto, I think?
regards
john
On Wed, Oct 02, 2024 at 01:15:00PM +0100, John Levon wrote:
>On Wed, Oct 02, 2024 at 01:00:33PM +0200, Martin Kletzander wrote:
>
>> > static int
>> > testDomainAttachHostDevice(testDriver *driver,
>> > virDomainObj *vm,
>> > @@ -10144,28 +10232,68 @@ testDomainAttachDeviceLive(virDomainObj *vm,
>> > testDriver *driver)
>> > {
>> > const char *alias = NULL;
>> > + int ret = -1;
>> >
>> > - if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) {
>> > + switch (dev->type) {
>> > + case VIR_DOMAIN_DEVICE_NET:
>> > + testDomainObjCheckNetTaint(vm, dev->data.net);
>> > + ret = testDomainAttachNetDevice(driver, vm, dev->data.net);
>> > + if (!ret) {
>>
>> if this is an integer where 0 is success and negative number is an error
>> we tend to check for (ret == 0). In this case it does not do much, but
>> in other instances it allows us to add different success return values
>> in the future. And keeping the same way of doing things makes it nicer
>> as consistent appearance is easier to read.
>
>Isn't it better to be consistent with the original code? This came directly from
>qemu_hotplug.c
>
Well, I could say that qemu's hotplug is a bit more complex as it uses
the @ret variable later on to decide whether to update the device list.
But this particular function in qemu_hotplug.c is very similar, only a
bit ugly, precisely because it uses mixture of (!ret) and (ret == 0).
>> > + alias = dev->data.hostdev->info->alias;
>> > + dev->data.hostdev = NULL;
>>
>> And since these two lines are the same for both code paths you can do
>> that after the switch when you error out early ...
>
>ditto, I think?
>
This one is my fault, I did not notice this accessing a different member
(duh!) so this needs to stay as it is anyway.
If you are fine with me removing the audit call, audit header include
and changing the "!ret" to "ret == 0", I'll fixup those changes and push
it as I'm fine with these patches given that modifications. Let me know.
Have a nice day,
Martin
>regards
>john
>
On Wed, Oct 02, 2024 at 03:55:58PM +0200, Martin Kletzander wrote: > If you are fine with me removing the audit call, audit header include > and changing the "!ret" to "ret == 0", I'll fixup those changes and push > it as I'm fine with these patches given that modifications. Let me know. That'd be super helpful, thanks! regards john
On Wed, Oct 02, 2024 at 03:31:21PM +0100, John Levon wrote: >On Wed, Oct 02, 2024 at 03:55:58PM +0200, Martin Kletzander wrote: > >> If you are fine with me removing the audit call, audit header include >> and changing the "!ret" to "ret == 0", I'll fixup those changes and push >> it as I'm fine with these patches given that modifications. Let me know. > >That'd be super helpful, thanks! > Pushed now, thanks. >regards >john >
© 2016 - 2024 Red Hat, Inc.