[libvirt] [PATCH 1/6] test_driver: implement virDomainAttachDeviceFlags
Ilias Stamatis
stamatis.iliass at gmail.com
Tue Sep 17 15:26:06 UTC 2019
On Tue, Sep 17, 2019 at 5:51 PM Daniel Henrique Barboza
<danielhb413 at gmail.com> wrote:
>
>
>
> On 8/1/19 9:54 AM, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis <stamatis.iliass at gmail.com>
> > ---
> > 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
>
> > + 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 at redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list