[libvirt] [PATCH 0/2] test_driver: add a disk device and implement
Ilias Stamatis (2): test_driver: add a disk device in the default config test_driver: implement virDomainGetFSInfo src/test/test_driver.c | 71 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) -- 2.22.0
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4b1f2724a0..1b1ff3003e 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -438,6 +438,11 @@ static const char *defaultConnXML = " <type>hvm</type>" " </os>" " <devices>" +" <disk type='file' device='disk'>" +" <source file='/guest/diskfile'/>" +" <target dev='vda' bus='virtio'/>" +" <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>" +" </disk>" " <interface type='network'>" " <mac address='aa:bb:cc:dd:ee:ff'/>" " <source network='default' bridge='virbr0'/>" -- 2.22.0
On Tue, Jun 25, 2019 at 11:58:06PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4b1f2724a0..1b1ff3003e 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -438,6 +438,11 @@ static const char *defaultConnXML = " <type>hvm</type>" " </os>" " <devices>" +" <disk type='file' device='disk'>" +" <source file='/guest/diskfile'/>"
In case we want to add another disk in the future, diskfile1 or diskimage1 would be IMO a better choice. Reviewed-by: Erik Skultety <eskultet@redhat.com>
+" <target dev='vda' bus='virtio'/>" +" <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>" +" </disk>" " <interface type='network'>" " <mac address='aa:bb:cc:dd:ee:ff'/>" " <source network='default' bridge='virbr0'/>" -- 2.22.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Always return / and /boot as the mount points imitating a default Fedora installation. Use the first disk found, otherwise if no disk device of type VIR_DOMAIN_DISK_DEVICE_DISK is present, return 0 mount points. Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1b1ff3003e..626449501f 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3298,6 +3298,71 @@ static int testDomainGetDiskErrors(virDomainPtr dom, return ret; } + + +static int +testDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags) +{ + size_t i; + char *name; + virDomainObjPtr vm; + virDomainFSInfoPtr *info_ret = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + *info = NULL; + + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (VIR_ALLOC_N(info_ret, 2) < 0 || VIR_ALLOC(info_ret[0]) < 0 || + VIR_ALLOC(info_ret[1]) < 0 || VIR_ALLOC(info_ret[0]->devAlias) < 0 || + VIR_ALLOC(info_ret[1]->devAlias) < 0) + goto cleanup; + + name = vm->def->disks[i]->dst; + + if (VIR_STRDUP(info_ret[0]->mountpoint, "/") < 0 || + VIR_STRDUP(info_ret[1]->mountpoint, "/boot") < 0 || + VIR_STRDUP(info_ret[0]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[1]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[0]->devAlias[0], name) < 0 || + VIR_STRDUP(info_ret[1]->devAlias[0], name) < 0 || + virAsprintf(&info_ret[0]->name, "%s1", name) < 0 || + virAsprintf(&info_ret[1]->name, "%s2", name) < 0) + goto cleanup; + + info_ret[0]->ndevAlias = info_ret[1]->ndevAlias = 1; + + VIR_STEAL_PTR(*info, info_ret); + + ret = 2; + goto cleanup; + } + } + + ret = 0; + + cleanup: + if (info_ret) { + virDomainFSInfoFree(info_ret[0]); + virDomainFSInfoFree(info_ret[1]); + VIR_FREE(info_ret); + } + + virDomainObjEndAPI(&vm); + return ret; +} + + static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, int *nparams) { @@ -7292,6 +7357,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */ .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */ .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */ + .domainGetFSInfo = testDomainGetFSInfo, /* 5.5.0 */ .domainGetSchedulerType = testDomainGetSchedulerType, /* 0.3.2 */ .domainGetSchedulerParameters = testDomainGetSchedulerParameters, /* 0.3.2 */ .domainGetSchedulerParametersFlags = testDomainGetSchedulerParametersFlags, /* 0.9.2 */ -- 2.22.0
On Tue, Jun 25, 2019 at 11:58:07PM +0200, Ilias Stamatis wrote:
Always return / and /boot as the mount points imitating a default Fedora installation. Use the first disk found, otherwise if no disk device of type VIR_DOMAIN_DISK_DEVICE_DISK is present, return 0 mount points.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1b1ff3003e..626449501f 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3298,6 +3298,71 @@ static int testDomainGetDiskErrors(virDomainPtr dom, return ret; }
+ + +static int +testDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags) +{ + size_t i; + char *name; + virDomainObjPtr vm; + virDomainFSInfoPtr *info_ret = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + *info = NULL; + + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (VIR_ALLOC_N(info_ret, 2) < 0 || VIR_ALLOC(info_ret[0]) < 0 || + VIR_ALLOC(info_ret[1]) < 0 || VIR_ALLOC(info_ret[0]->devAlias) < 0 || + VIR_ALLOC(info_ret[1]->devAlias) < 0)
^these should be on separate lines to enhance readability.
+ goto cleanup; + + name = vm->def->disks[i]->dst; + + if (VIR_STRDUP(info_ret[0]->mountpoint, "/") < 0 || + VIR_STRDUP(info_ret[1]->mountpoint, "/boot") < 0 || + VIR_STRDUP(info_ret[0]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[1]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[0]->devAlias[0], name) < 0 || + VIR_STRDUP(info_ret[1]->devAlias[0], name) < 0 || + virAsprintf(&info_ret[0]->name, "%s1", name) < 0 || + virAsprintf(&info_ret[1]->name, "%s2", name) < 0) + goto cleanup;
Okay, alternatively we could have 2 'if' clauses for each mountpoint, this is strictly a personal preference, I think it could read even better if the mountpoints were logically separated, but I won't insist on that. Let me know whether you're okay with the proposed tweaks. Reviewed-by: Erik Skultety <eskultet@redhat.com>
+ + info_ret[0]->ndevAlias = info_ret[1]->ndevAlias = 1; + + VIR_STEAL_PTR(*info, info_ret); + + ret = 2; + goto cleanup; + } + } + + ret = 0; + + cleanup: + if (info_ret) { + virDomainFSInfoFree(info_ret[0]); + virDomainFSInfoFree(info_ret[1]); + VIR_FREE(info_ret); + } + + virDomainObjEndAPI(&vm); + return ret; +} + + static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, int *nparams) { @@ -7292,6 +7357,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */ .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */ .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */ + .domainGetFSInfo = testDomainGetFSInfo, /* 5.5.0 */ .domainGetSchedulerType = testDomainGetSchedulerType, /* 0.3.2 */ .domainGetSchedulerParameters = testDomainGetSchedulerParameters, /* 0.3.2 */ .domainGetSchedulerParametersFlags = testDomainGetSchedulerParametersFlags, /* 0.9.2 */ -- 2.22.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 2, 2019 at 2:41 PM Erik Skultety <eskultet@redhat.com> wrote:
On Tue, Jun 25, 2019 at 11:58:07PM +0200, Ilias Stamatis wrote:
Always return / and /boot as the mount points imitating a default Fedora installation. Use the first disk found, otherwise if no disk device of type VIR_DOMAIN_DISK_DEVICE_DISK is present, return 0 mount points.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1b1ff3003e..626449501f 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3298,6 +3298,71 @@ static int testDomainGetDiskErrors(virDomainPtr dom, return ret; }
+ + +static int +testDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags) +{ + size_t i; + char *name; + virDomainObjPtr vm; + virDomainFSInfoPtr *info_ret = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + *info = NULL; + + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (VIR_ALLOC_N(info_ret, 2) < 0 || VIR_ALLOC(info_ret[0]) < 0 || + VIR_ALLOC(info_ret[1]) < 0 || VIR_ALLOC(info_ret[0]->devAlias) < 0 || + VIR_ALLOC(info_ret[1]->devAlias) < 0)
^these should be on separate lines to enhance readability.
Sure. I'm not sure if there are certain guidelines for that, but I think nothing is mentioned in the "Contributor guidelines". Many times I'm unsure how to split long lines, especially on if statements.
+ goto cleanup; + + name = vm->def->disks[i]->dst; + + if (VIR_STRDUP(info_ret[0]->mountpoint, "/") < 0 || + VIR_STRDUP(info_ret[1]->mountpoint, "/boot") < 0 || + VIR_STRDUP(info_ret[0]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[1]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[0]->devAlias[0], name) < 0 || + VIR_STRDUP(info_ret[1]->devAlias[0], name) < 0 || + virAsprintf(&info_ret[0]->name, "%s1", name) < 0 || + virAsprintf(&info_ret[1]->name, "%s2", name) < 0) + goto cleanup;
Okay, alternatively we could have 2 'if' clauses for each mountpoint, this is strictly a personal preference, I think it could read even better if the mountpoints were logically separated, but I won't insist on that.
That makes sense and is perfectly fine with me.
Let me know whether you're okay with the proposed tweaks.
Will you edit these before merging or do you want me to re-send it?
Reviewed-by: Erik Skultety <eskultet@redhat.com>
+ + info_ret[0]->ndevAlias = info_ret[1]->ndevAlias = 1; + + VIR_STEAL_PTR(*info, info_ret); + + ret = 2; + goto cleanup; + } + } + + ret = 0; + + cleanup: + if (info_ret) { + virDomainFSInfoFree(info_ret[0]); + virDomainFSInfoFree(info_ret[1]); + VIR_FREE(info_ret); + } + + virDomainObjEndAPI(&vm); + return ret; +} + + static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, int *nparams) { @@ -7292,6 +7357,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */ .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */ .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */ + .domainGetFSInfo = testDomainGetFSInfo, /* 5.5.0 */ .domainGetSchedulerType = testDomainGetSchedulerType, /* 0.3.2 */ .domainGetSchedulerParameters = testDomainGetSchedulerParameters, /* 0.3.2 */ .domainGetSchedulerParametersFlags = testDomainGetSchedulerParametersFlags, /* 0.9.2 */ -- 2.22.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 02, 2019 at 02:49:02PM +0200, Ilias Stamatis wrote:
On Tue, Jul 2, 2019 at 2:41 PM Erik Skultety <eskultet@redhat.com> wrote:
On Tue, Jun 25, 2019 at 11:58:07PM +0200, Ilias Stamatis wrote:
Always return / and /boot as the mount points imitating a default Fedora installation. Use the first disk found, otherwise if no disk device of type VIR_DOMAIN_DISK_DEVICE_DISK is present, return 0 mount points.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1b1ff3003e..626449501f 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3298,6 +3298,71 @@ static int testDomainGetDiskErrors(virDomainPtr dom, return ret; }
+ + +static int +testDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags) +{ + size_t i; + char *name; + virDomainObjPtr vm; + virDomainFSInfoPtr *info_ret = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + *info = NULL; + + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (VIR_ALLOC_N(info_ret, 2) < 0 || VIR_ALLOC(info_ret[0]) < 0 || + VIR_ALLOC(info_ret[1]) < 0 || VIR_ALLOC(info_ret[0]->devAlias) < 0 || + VIR_ALLOC(info_ret[1]->devAlias) < 0)
^these should be on separate lines to enhance readability.
Sure. I'm not sure if there are certain guidelines for that, but I think nothing is mentioned in the "Contributor guidelines".
This is solely because of readability, we should have something like that in the guidelines.
Many times I'm unsure how to split long lines, especially on if statements.
+ goto cleanup; + + name = vm->def->disks[i]->dst; + + if (VIR_STRDUP(info_ret[0]->mountpoint, "/") < 0 || + VIR_STRDUP(info_ret[1]->mountpoint, "/boot") < 0 || + VIR_STRDUP(info_ret[0]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[1]->fstype, "ext4") < 0 || + VIR_STRDUP(info_ret[0]->devAlias[0], name) < 0 || + VIR_STRDUP(info_ret[1]->devAlias[0], name) < 0 || + virAsprintf(&info_ret[0]->name, "%s1", name) < 0 || + virAsprintf(&info_ret[1]->name, "%s2", name) < 0) + goto cleanup;
Okay, alternatively we could have 2 'if' clauses for each mountpoint, this is strictly a personal preference, I think it could read even better if the mountpoints were logically separated, but I won't insist on that.
That makes sense and is perfectly fine with me.
Let me know whether you're okay with the proposed tweaks.
Will you edit these before merging or do you want me to re-send it?
No need, I'll do that before merging. Erik
Reviewed-by: Erik Skultety <eskultet@redhat.com>
+ + info_ret[0]->ndevAlias = info_ret[1]->ndevAlias = 1; + + VIR_STEAL_PTR(*info, info_ret); + + ret = 2; + goto cleanup; + } + } + + ret = 0; + + cleanup: + if (info_ret) { + virDomainFSInfoFree(info_ret[0]); + virDomainFSInfoFree(info_ret[1]); + VIR_FREE(info_ret); + } + + virDomainObjEndAPI(&vm); + return ret; +} + + static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, int *nparams) { @@ -7292,6 +7357,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */ .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */ .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */ + .domainGetFSInfo = testDomainGetFSInfo, /* 5.5.0 */ .domainGetSchedulerType = testDomainGetSchedulerType, /* 0.3.2 */ .domainGetSchedulerParameters = testDomainGetSchedulerParameters, /* 0.3.2 */ .domainGetSchedulerParametersFlags = testDomainGetSchedulerParametersFlags, /* 0.9.2 */ -- 2.22.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Erik Skultety -
Ilias Stamatis