[libvirt] [PATCH 1/6] test_driver: implement virDomainAttachDeviceFlags
Daniel Henrique Barboza
danielhb413 at gmail.com
Tue Sep 17 15:28:59 UTC 2019
On 9/17/19 12:26 PM, Ilias Stamatis wrote:
> 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
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 at redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list