Re: [PATCH 1/6] test_driver: implement virDomainAttachDeviceFlags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 9/17/19 12:26 PM, Ilias Stamatis wrote:
On Tue, Sep 17, 2019 at 5:51 PM Daniel Henrique Barboza
<danielhb413@xxxxxxxxx> wrote:


On 8/1/19 9:54 AM, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@xxxxxxxxx>
---
   src/test/test_driver.c | 290 +++++++++++++++++++++++++++++++++++++++++
   1 file changed, 290 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index aae9875194..c8aad6a0bb 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4013,6 +4013,295 @@ static int testDomainUndefine(virDomainPtr domain)
       return testDomainUndefineFlags(domain, 0);
   }

+
+static int
+testDomainAttachDeviceLiveAndConfig(virDomainDefPtr vmdef,
+                                    virDomainDeviceDefPtr dev)
+{
+    virDomainDiskDefPtr disk;
+    virDomainNetDefPtr net;
+    virDomainHostdevDefPtr hostdev;
+    virDomainControllerDefPtr controller;
+    virDomainHostdevDefPtr found;
+    virDomainLeaseDefPtr lease;
+    virDomainFSDefPtr fs;
+    virDomainRedirdevDefPtr redirdev;
+    virDomainShmemDefPtr shmem;
+    char mac[VIR_MAC_STRING_BUFLEN];
+
+    switch (dev->type) {
+        case VIR_DOMAIN_DEVICE_DISK:
+            disk = dev->data.disk;
+            if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) {
+                virReportError(VIR_ERR_INVALID_ARG,
+                               _("target %s already exists."), disk->dst);
+                return -1;
+            }
+
+            if (virDomainDiskInsert(vmdef, disk) < 0)
+                return -1;
+
+            dev->data.disk = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_CONTROLLER:
+            controller = dev->data.controller;
+            if (controller->idx != -1 &&
+                virDomainControllerFind(vmdef, controller->type,
+                                        controller->idx) >= 0) {
+                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                               _("Target already exists"));
+                return -1;
+            }
+
+            if (virDomainControllerInsert(vmdef, controller) < 0)
+                return -1;
+
+            dev->data.controller = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_NET:
+            net = dev->data.net;
+            if (virDomainHasNet(vmdef, net)) {
+                virReportError(VIR_ERR_INVALID_ARG,
+                               _("network device with mac %s already exists"),
+                               virMacAddrFormat(&net->mac, mac));
+                return -1;
+            }
+
+            if (virDomainNetInsert(vmdef, net))
+                return -1;
+
+            dev->data.net = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_HOSTDEV:
+            hostdev = dev->data.hostdev;
+            if (virDomainHostdevFind(vmdef, hostdev, &found) >= 0) {
+                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                               _("device is already in the domain configuration"));
+                return -1;
+            }
+
+            if (virDomainHostdevInsert(vmdef, hostdev) < 0)
+                return -1;
+
+            dev->data.hostdev = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_LEASE:
+            lease = dev->data.lease;
+            if (virDomainLeaseIndex(vmdef, lease) >= 0) {
+                virReportError(VIR_ERR_OPERATION_INVALID,
+                               _("Lease %s in lockspace %s already exists"),
+                               lease->key, NULLSTR(lease->lockspace));
+                return -1;
+            }
+
+            if (virDomainLeaseInsert(vmdef, lease) < 0)
+                return -1;
+
+            dev->data.lease = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_FS:
+            fs = dev->data.fs;
+            if (virDomainFSIndexByName(vmdef, fs->dst) >= 0) {
+                virReportError(VIR_ERR_OPERATION_INVALID,
+                             "%s", _("Target already exists"));
+                return -1;
+            }
+
+            if (virDomainFSInsert(vmdef, fs) < 0)
+                return -1;
+
+            dev->data.fs = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_RNG:
+            if (dev->data.rng->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+                virDomainDefHasDeviceAddress(vmdef, &dev->data.rng->info)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("a device with the same address already exists "));
+                return -1;
+            }
+
+            if (VIR_APPEND_ELEMENT(vmdef->rngs, vmdef->nrngs, dev->data.rng) < 0)
+                return -1;
+
+            dev->data.rng = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_MEMORY:
+            if (vmdef->nmems == vmdef->mem.memory_slots) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("no free memory device slot available"));
+                return -1;
+            }
+
+            vmdef->mem.cur_balloon += dev->data.memory->size;
+            if (virDomainMemoryInsert(vmdef, dev->data.memory) < 0)
+                return -1;
+
+            dev->data.memory = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_REDIRDEV:
+            redirdev = dev->data.redirdev;
+
+            if (VIR_APPEND_ELEMENT(vmdef->redirdevs, vmdef->nredirdevs, redirdev) < 0)
+                return -1;
+
+            dev->data.redirdev = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_SHMEM:
+            shmem = dev->data.shmem;
+            if (virDomainShmemDefFind(vmdef, shmem) >= 0) {
+                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                               _("device is already in the domain configuration"));
+                return -1;
+            }
+
+            if (virDomainShmemDefInsert(vmdef, shmem) < 0)
+                return -1;
+
+            dev->data.shmem = NULL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_WATCHDOG:
+            if (vmdef->watchdog) {
+                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                               _("domain already has a watchdog"));
+                return -1;
+            }
+            VIR_STEAL_PTR(vmdef->watchdog, dev->data.watchdog);
+            break;
+
+        case VIR_DOMAIN_DEVICE_INPUT:
+            if (VIR_APPEND_ELEMENT(vmdef->inputs, vmdef->ninputs, dev->data.input) < 0)
+                return -1;
+            break;
+
+        case VIR_DOMAIN_DEVICE_VSOCK:
+            if (vmdef->vsock) {
+                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                               _("domain already has a vsock device"));
+                return -1;
+            }
+            VIR_STEAL_PTR(vmdef->vsock, dev->data.vsock);
+            break;
+
+        default:
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("persistent attach of device is not supported"));
+            return -1;
+    }
+
+    return 0;
+}
+
+
+typedef enum {
+    TEST_DEVICE_ATTACH = 0,
+    TEST_DEVICE_DETACH,
+    TEST_DEVICE_UPDATE,
+} virTestDeviceOperation;
+
+
+static int
+testDomainDeviceOperation(testDriverPtr driver,
+                          virTestDeviceOperation operation,
+                          const char *xml,
+                          const char *alias,
+                          virDomainDefPtr def)
+{
+    virDomainDeviceDefPtr dev = NULL;
+    unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+    int ret = -1;
+
+    if (operation == TEST_DEVICE_DETACH)
+        parse_flags |= VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
+
+    if (xml) {
+        if (!(dev = virDomainDeviceDefParse(xml, def,
+                                            driver->caps, driver->xmlopt,
+                                            parse_flags)))

This API now has an extra parameter "parseOpaque" right after the
parse_flags.

Using NULL in this parameter can be a way out, assuming nothing breaks of
course.
Hello,

There's a v2 of this series that fixes that IIRC:
https://www.redhat.com/archives/libvir-list/2019-August/msg00535.html

Didn't notice it. Thanks for pointing it out. I'll have a look.


DHB

+            goto cleanup;
+    } else if (alias) {
+        if (VIR_ALLOC(dev) < 0 || virDomainDefFindDevice(def, alias, dev, true) < 0)
+            goto cleanup;
+    }
+
+    switch (operation) {
+    case TEST_DEVICE_ATTACH:
+        if (testDomainAttachDeviceLiveAndConfig(def, dev) < 0)
+            goto cleanup;
+        break;
+    case TEST_DEVICE_DETACH:
+        break;
+    case TEST_DEVICE_UPDATE:
+        break;
+    }
+
+    ret = 0;
+ cleanup:
+    if (xml)
+        virDomainDeviceDefFree(dev);
+    else
+        VIR_FREE(dev);
+    return ret;
+}
+
+
+static int
+testDomainAttachDetachUpdateDevice(virDomainPtr dom,
+                                   virTestDeviceOperation operation,
+                                   const char *xml,
+                                   const char *alias,
+                                   unsigned int flags)
+{
+    testDriverPtr driver = dom->conn->privateData;
+    virDomainObjPtr vm = NULL;
+    virDomainDefPtr def;
+    virDomainDefPtr persistentDef;
+    int ret = -1;
+
+    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+                  VIR_DOMAIN_AFFECT_CONFIG, -1);
+
+    if (!(vm = testDomObjFromDomain(dom)))
+        goto cleanup;
+
+    if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
+        goto cleanup;
+
+    if (persistentDef) {
+        if (testDomainDeviceOperation(driver, operation, xml, alias, persistentDef) < 0)
+            goto cleanup;
+    }
+
+    if (def) {
+        if (testDomainDeviceOperation(driver, operation, xml, alias, def) < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return ret;
+}
+
+
+static int
+testDomainAttachDeviceFlags(virDomainPtr dom,
+                            const char *xml,
+                            unsigned int flags)
+{
+    return testDomainAttachDetachUpdateDevice(dom, TEST_DEVICE_ATTACH,
+                                              xml, NULL, flags);
+}
+
Compiler wasn't happy with this function:


test/test_driver.c:4767:1: error: 'testDomainAttachDeviceFlags' defined
but not used [-Werror=unused-function]
   4767 | testDomainAttachDeviceFlags(virDomainPtr dom,
        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


Since you're using it in patch 02 you can move it there to avoid this error.
That's weird since it's used below as far as I can see.

Problem is, doing that, you'll get the same error in
testDomainAttachDetachUpdateDevice.
If you erase this one too you'll have the warning with
testDomainDeviceOperation.
And then, with testDomainAttachDeviceLiveAndConfig.

Basically this first patch introduces static functions that aren't being
used anywhere
else, thus the compiler will not play ball. A quick solution is to merge
this patch with
patch 02.



Thanks,


DHB


   static int testDomainGetAutostart(virDomainPtr domain,
                                     int *autostart)
   {
@@ -8659,6 +8948,7 @@ static virHypervisorDriver testHypervisorDriver = {
       .domainDefineXMLFlags = testDomainDefineXMLFlags, /* 1.2.12 */
       .domainUndefine = testDomainUndefine, /* 0.1.11 */
       .domainUndefineFlags = testDomainUndefineFlags, /* 0.9.4 */
+    .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 5.7.0 */
It is used here.

Thanks,
Ilias

       .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
       .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
       .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux