| Message ID | 1504190408-11143-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tests: Enable the drive_del test also on s390x | expand |
> static void test_drive_del_device_del(void) > { > + char *args; > + > /* Start with a drive used by a device that unplugs instantaneously */ > - qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw" > - " -device virtio-scsi-pci" > - " -device scsi-hd,drive=drive0,id=dev0"); > + args = g_strdup_printf("-drive if=none,id=drive0,file=null-co://,format=raw" > + " -device virtio-scsi-%s" > + " -device scsi-hd,drive=drive0,id=dev0", Would look better with the spaces at the end of the previous line (so all "-device" are aligned), but just my taste. > + qvirtio_get_dev_type()); > + qtest_start(args); > > /* > * Delete the drive, and then the device > @@ -104,6 +109,7 @@ static void test_drive_del_device_del(void) > device_del(); > > qtest_end(); > + g_free(args); > } > > int main(int argc, char **argv) > @@ -114,9 +120,10 @@ int main(int argc, char **argv) > > qtest_add_func("/drive_del/without-dev", test_drive_without_dev); > > - /* TODO I guess any arch with PCI would do */ > + /* TODO I guess any arch with a hot-pluggable virtio bus would do */ > if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64") || > - !strcmp(arch, "ppc") || !strcmp(arch, "ppc64")) { > + !strcmp(arch, "ppc") || !strcmp(arch, "ppc64") || > + !strcmp(arch, "s390x")) { > qtest_add_func("/drive_del/after_failed_device_add", > test_after_failed_device_add); > qtest_add_func("/blockdev/drive_del_device_del", > diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c > index 9880a69..0879a62 100644 > --- a/tests/libqos/virtio.c > +++ b/tests/libqos/virtio.c > @@ -339,3 +339,20 @@ void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx) > /* vq->avail->used_event */ > writew(vq->avail + 4 + (2 * vq->size), idx); > } > + > +/* > + * qvirtio_get_dev_type: > + * Returns: the preferred virtio bus/device type for the current architecture. > + */ > +const char *qvirtio_get_dev_type(void) > +{ > + const char *arch = qtest_get_arch(); > + > + if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) { > + return "device"; /* for virtio-mmio */ > + } else if (g_str_equal(arch, "s390x")) { > + return "ccw"; > + } else { > + return "pci"; > + } You could drop the else case and do it unconditionally. > +} > diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h > index 8fbcd18..0a04740 100644 > --- a/tests/libqos/virtio.h > +++ b/tests/libqos/virtio.h > @@ -143,4 +143,7 @@ void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head); > bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx); > > void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx); > + > +const char *qvirtio_get_dev_type(void); > + > #endif > Looks good to me! Reviewed-by: David Hildenbrand <david@redhat.com>
David Hildenbrand <david@redhat.com> writes: >> static void test_drive_del_device_del(void) >> { >> + char *args; >> + >> /* Start with a drive used by a device that unplugs instantaneously */ >> - qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw" >> - " -device virtio-scsi-pci" >> - " -device scsi-hd,drive=drive0,id=dev0"); >> + args = g_strdup_printf("-drive if=none,id=drive0,file=null-co://,format=raw" >> + " -device virtio-scsi-%s" >> + " -device scsi-hd,drive=drive0,id=dev0", > > Would look better with the spaces at the end of the previous line (so > all "-device" are aligned), but just my taste. The -device *are* aligned, but I get what you mean. The advantage of leading rather than trailing space is that the intention "this is still the same string" is locally obvious both at the first part's end (no comma) and at the second part's beginning (leading space). >> + qvirtio_get_dev_type()); >> + qtest_start(args); >> >> /* >> * Delete the drive, and then the device [...]
On 01.09.2017 11:10, Markus Armbruster wrote: > David Hildenbrand <david@redhat.com> writes: > >>> static void test_drive_del_device_del(void) >>> { >>> + char *args; >>> + >>> /* Start with a drive used by a device that unplugs instantaneously */ >>> - qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw" >>> - " -device virtio-scsi-pci" >>> - " -device scsi-hd,drive=drive0,id=dev0"); >>> + args = g_strdup_printf("-drive if=none,id=drive0,file=null-co://,format=raw" >>> + " -device virtio-scsi-%s" >>> + " -device scsi-hd,drive=drive0,id=dev0", >> >> Would look better with the spaces at the end of the previous line (so >> all "-device" are aligned), but just my taste. > > The -device *are* aligned, but I get what you mean. > > The advantage of leading rather than trailing space is that the > intention "this is still the same string" is locally obvious both at the > first part's end (no comma) and at the second part's beginning (leading > space). I don't mind either way, but in this case, I think I'd prefer to keep the original formatting with the space at the beginning, for the following two reasons: - The first line is already hitting the 80 columns limit, and I want to avoid complaints from checkpatch.pl - The original author formatted it that way. Thomas
On Fri, 1 Sep 2017 12:39:49 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 01.09.2017 11:10, Markus Armbruster wrote: > > David Hildenbrand <david@redhat.com> writes: > > > >>> static void test_drive_del_device_del(void) > >>> { > >>> + char *args; > >>> + > >>> /* Start with a drive used by a device that unplugs instantaneously */ > >>> - qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw" > >>> - " -device virtio-scsi-pci" > >>> - " -device scsi-hd,drive=drive0,id=dev0"); > >>> + args = g_strdup_printf("-drive if=none,id=drive0,file=null-co://,format=raw" > >>> + " -device virtio-scsi-%s" > >>> + " -device scsi-hd,drive=drive0,id=dev0", > >> > >> Would look better with the spaces at the end of the previous line (so > >> all "-device" are aligned), but just my taste. > > > > The -device *are* aligned, but I get what you mean. > > > > The advantage of leading rather than trailing space is that the > > intention "this is still the same string" is locally obvious both at the > > first part's end (no comma) and at the second part's beginning (leading > > space). > > I don't mind either way, but in this case, I think I'd prefer to keep > the original formatting with the space at the beginning, for the > following two reasons: > > - The first line is already hitting the 80 columns limit, and I want to > avoid complaints from checkpatch.pl > - The original author formatted it that way. I'd keep it that way as well.
On Thu, 31 Aug 2017 16:40:08 +0200 Thomas Huth <thuth@redhat.com> wrote: > We can use the drive_del test on s390x, too, to check that adding and > deleting also works fine with the virtio-ccw bus. But we have to make > sure that we use the devices with the "-ccw" suffix instead of the > "-pci" suffix for the virtio-ccw transport on s390x. Introduce a helper > function called qvirtio_get_dev_type() that returns the correct string > for the current architecture. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > I'm just sending a patch for this test now to get some feedback whether > I'm on the right track now. If this approach with qvirtio_get_dev_type() > gets accepted, I'll continue converting the other virtio tests, too. Looks good. > > tests/Makefile.include | 3 ++- > tests/drive_del-test.c | 25 ++++++++++++++++--------- > tests/libqos/virtio.c | 17 +++++++++++++++++ > tests/libqos/virtio.h | 3 +++ > 4 files changed, 38 insertions(+), 10 deletions(-) > > diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c > index 9880a69..0879a62 100644 > --- a/tests/libqos/virtio.c > +++ b/tests/libqos/virtio.c > @@ -339,3 +339,20 @@ void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx) > /* vq->avail->used_event */ > writew(vq->avail + 4 + (2 * vq->size), idx); > } > + > +/* > + * qvirtio_get_dev_type: > + * Returns: the preferred virtio bus/device type for the current architecture. > + */ > +const char *qvirtio_get_dev_type(void) > +{ > + const char *arch = qtest_get_arch(); > + > + if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) { > + return "device"; /* for virtio-mmio */ Would not mind a comment from someone familiar with virtio-mmio, though. > + } else if (g_str_equal(arch, "s390x")) { > + return "ccw"; > + } else { > + return "pci"; > + } > +} I'll take this, unless someone complains.
On 09/01/2017 05:39 AM, Thomas Huth wrote: >>>> /* Start with a drive used by a device that unplugs instantaneously */ >>>> - qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw" >>>> - " -device virtio-scsi-pci" >>>> - " -device scsi-hd,drive=drive0,id=dev0"); >>>> + args = g_strdup_printf("-drive if=none,id=drive0,file=null-co://,format=raw" >>>> + " -device virtio-scsi-%s" >>>> + " -device scsi-hd,drive=drive0,id=dev0", >>> >>> Would look better with the spaces at the end of the previous line (so >>> all "-device" are aligned), but just my taste. >> >> The -device *are* aligned, but I get what you mean. >> >> The advantage of leading rather than trailing space is that the >> intention "this is still the same string" is locally obvious both at the >> first part's end (no comma) and at the second part's beginning (leading >> space). > > I don't mind either way, but in this case, I think I'd prefer to keep > the original formatting with the space at the beginning, for the > following two reasons: > > - The first line is already hitting the 80 columns limit, and I want to > avoid complaints from checkpatch.pl > - The original author formatted it that way. As it is, I got annoyed by the fact that we had so much strdup() of args to pass to qtest_init/qtest_start(), so my latest series reformats the code yet another way: https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg00308.html Prone to cause some fun conflict resolution if my cleanups take too long to get in the tree...
On Fri, 1 Sep 2017 15:43:00 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 31 Aug 2017 16:40:08 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > We can use the drive_del test on s390x, too, to check that adding and > > deleting also works fine with the virtio-ccw bus. But we have to make > > sure that we use the devices with the "-ccw" suffix instead of the > > "-pci" suffix for the virtio-ccw transport on s390x. Introduce a helper > > function called qvirtio_get_dev_type() that returns the correct string > > for the current architecture. > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > I'm just sending a patch for this test now to get some feedback whether > > I'm on the right track now. If this approach with qvirtio_get_dev_type() > > gets accepted, I'll continue converting the other virtio tests, too. > > Looks good. > > > > > tests/Makefile.include | 3 ++- > > tests/drive_del-test.c | 25 ++++++++++++++++--------- > > tests/libqos/virtio.c | 17 +++++++++++++++++ > > tests/libqos/virtio.h | 3 +++ > > 4 files changed, 38 insertions(+), 10 deletions(-) > > > > > diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c > > index 9880a69..0879a62 100644 > > --- a/tests/libqos/virtio.c > > +++ b/tests/libqos/virtio.c > > @@ -339,3 +339,20 @@ void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx) > > /* vq->avail->used_event */ > > writew(vq->avail + 4 + (2 * vq->size), idx); > > } > > + > > +/* > > + * qvirtio_get_dev_type: > > + * Returns: the preferred virtio bus/device type for the current architecture. > > + */ > > +const char *qvirtio_get_dev_type(void) > > +{ > > + const char *arch = qtest_get_arch(); > > + > > + if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) { > > + return "device"; /* for virtio-mmio */ > > Would not mind a comment from someone familiar with virtio-mmio, though. > > > + } else if (g_str_equal(arch, "s390x")) { > > + return "ccw"; > > + } else { > > + return "pci"; > > + } > > +} > > I'll take this, unless someone complains. Nobody complained, so I applied this.
diff --git a/tests/Makefile.include b/tests/Makefile.include index f08b741..391c7a7 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -364,6 +364,7 @@ check-qtest-s390x-$(CONFIG_SLIRP) += tests/pxe-test$(EXESUF) check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF) check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF) check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF) +check-qtest-s390x-y += tests/drive_del-test$(EXESUF) check-qtest-generic-y += tests/qom-test$(EXESUF) check-qtest-generic-y += tests/test-hmp$(EXESUF) @@ -762,7 +763,7 @@ tests/display-vga-test$(EXESUF): tests/display-vga-test.o tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o tests/qom-test$(EXESUF): tests/qom-test.o tests/test-hmp$(EXESUF): tests/test-hmp.o -tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y) +tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-virtio-obj-y) tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y) tests/nvme-test$(EXESUF): tests/nvme-test.o tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c index 2175139..c9ac997 100644 --- a/tests/drive_del-test.c +++ b/tests/drive_del-test.c @@ -12,6 +12,7 @@ #include "qemu/osdep.h" #include "libqtest.h" +#include "libqos/virtio.h" static void drive_add(void) { @@ -65,14 +66,14 @@ static void test_after_failed_device_add(void) qtest_start("-drive if=none,id=drive0"); - /* Make device_add fail. If this leaks the virtio-blk-pci device then a + /* Make device_add fail. If this leaks the virtio-blk device then a * reference to drive0 will also be held (via qdev properties). */ response = qmp("{'execute': 'device_add'," " 'arguments': {" - " 'driver': 'virtio-blk-pci'," + " 'driver': 'virtio-blk-%s'," " 'drive': 'drive0'" - "}}"); + "}}", qvirtio_get_dev_type()); g_assert(response); error = qdict_get_qdict(response, "error"); g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, "GenericError"); @@ -82,7 +83,7 @@ static void test_after_failed_device_add(void) drive_del(); /* Try to re-add the drive. This fails with duplicate IDs if a leaked - * virtio-blk-pci exists that holds a reference to the old drive0. + * virtio-blk device exists that holds a reference to the old drive0. */ drive_add(); @@ -91,10 +92,14 @@ static void test_after_failed_device_add(void) static void test_drive_del_device_del(void) { + char *args; + /* Start with a drive used by a device that unplugs instantaneously */ - qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw" - " -device virtio-scsi-pci" - " -device scsi-hd,drive=drive0,id=dev0"); + args = g_strdup_printf("-drive if=none,id=drive0,file=null-co://,format=raw" + " -device virtio-scsi-%s" + " -device scsi-hd,drive=drive0,id=dev0", + qvirtio_get_dev_type()); + qtest_start(args); /* * Delete the drive, and then the device @@ -104,6 +109,7 @@ static void test_drive_del_device_del(void) device_del(); qtest_end(); + g_free(args); } int main(int argc, char **argv) @@ -114,9 +120,10 @@ int main(int argc, char **argv) qtest_add_func("/drive_del/without-dev", test_drive_without_dev); - /* TODO I guess any arch with PCI would do */ + /* TODO I guess any arch with a hot-pluggable virtio bus would do */ if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64") || - !strcmp(arch, "ppc") || !strcmp(arch, "ppc64")) { + !strcmp(arch, "ppc") || !strcmp(arch, "ppc64") || + !strcmp(arch, "s390x")) { qtest_add_func("/drive_del/after_failed_device_add", test_after_failed_device_add); qtest_add_func("/blockdev/drive_del_device_del", diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c index 9880a69..0879a62 100644 --- a/tests/libqos/virtio.c +++ b/tests/libqos/virtio.c @@ -339,3 +339,20 @@ void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx) /* vq->avail->used_event */ writew(vq->avail + 4 + (2 * vq->size), idx); } + +/* + * qvirtio_get_dev_type: + * Returns: the preferred virtio bus/device type for the current architecture. + */ +const char *qvirtio_get_dev_type(void) +{ + const char *arch = qtest_get_arch(); + + if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) { + return "device"; /* for virtio-mmio */ + } else if (g_str_equal(arch, "s390x")) { + return "ccw"; + } else { + return "pci"; + } +} diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h index 8fbcd18..0a04740 100644 --- a/tests/libqos/virtio.h +++ b/tests/libqos/virtio.h @@ -143,4 +143,7 @@ void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head); bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx); void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx); + +const char *qvirtio_get_dev_type(void); + #endif
We can use the drive_del test on s390x, too, to check that adding and deleting also works fine with the virtio-ccw bus. But we have to make sure that we use the devices with the "-ccw" suffix instead of the "-pci" suffix for the virtio-ccw transport on s390x. Introduce a helper function called qvirtio_get_dev_type() that returns the correct string for the current architecture. Signed-off-by: Thomas Huth <thuth@redhat.com> --- I'm just sending a patch for this test now to get some feedback whether I'm on the right track now. If this approach with qvirtio_get_dev_type() gets accepted, I'll continue converting the other virtio tests, too. tests/Makefile.include | 3 ++- tests/drive_del-test.c | 25 ++++++++++++++++--------- tests/libqos/virtio.c | 17 +++++++++++++++++ tests/libqos/virtio.h | 3 +++ 4 files changed, 38 insertions(+), 10 deletions(-)