Re: [PATCH v2 3/4] tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer
On 01/12/2020 20.10, Philippe Mathieu-Daudé wrote:
> Add a reproducer which triggers (without the previous patch):
>
> $ make check-qtest-x86_64
> Running test qtest-x86_64/fuzz-test
> qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
> `cdb_len > 0 && scsi_cdb_length(cdb) <= cdb_len' failed.
> tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
> (Aborted) (core dumped)
> ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
>
> Signed-off-by: Alexander Bulekov
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/qtest/fuzz-test.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 87b72307a5b..31f90cfb4fc 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -48,6 +48,23 @@ static void
> test_lp1878642_pci_bus_get_irq_level_assert(void)
> qtest_quit(s);
> }
>
> +static void test_megasas_cdb_len_zero(void)
> +{
> +QTestState *s;
> +
> +s = qtest_init("-M pc -nodefaults "
> + "-device megasas-gen2 -device scsi-cd,drive=null0 "
> + "-blockdev
> driver=null-co,read-zeroes=on,node-name=null0");
> +
> +qtest_outl(s, 0xcf8, 0x80001011);
> +qtest_outb(s, 0xcfc, 0xbb);
> +qtest_outl(s, 0xcf8, 0x80001002);
> +qtest_outl(s, 0xcfc, 0xf3ff2966);
> +qtest_writeb(s, 0x4600, 0x03);
> +qtest_outw(s, 0xbb40, 0x460b);
> +qtest_quit(s);
> +}
> +
> int main(int argc, char **argv)
> {
> const char *arch = qtest_get_arch();
> @@ -59,6 +76,8 @@ int main(int argc, char **argv)
> test_lp1878263_megasas_zero_iov_cnt);
> qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
> test_lp1878642_pci_bus_get_irq_level_assert);
> +qtest_add_func("fuzz/test_megasas_cdb_len_zero",
> + test_megasas_cdb_len_zero);
> }
>
> return g_test_run();
>
Acked-by: Thomas Huth
Re: [PATCH] qemu-nbd: Fix a memleak in nbd_client_thread()
On 2020/12/2 4:15, Eric Blake wrote:
> On 12/1/20 12:13 AM, Alex Chen wrote:
>> When the qio_channel_socket_connect_sync() fails
>> we should goto 'out_socket' label to free the 'sioc' instead of
>> goto 'out' label.
>> In addition, now the 'out' label is useless, delete it.
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Alex Chen
>> ---
>> qemu-nbd.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 47587a709e..643b0777c0 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -275,7 +275,7 @@ static void *nbd_client_thread(void *arg)
>> saddr,
>> _error) < 0) {
>> error_report_err(local_error);
>> -goto out;
>> +goto out_socket;
>> }
>>
>> ret = nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
>> @@ -325,7 +325,6 @@ out_fd:
>> close(fd);
>> out_socket:
>> object_unref(OBJECT(sioc));
>> -out:
>> g_free(info.name);
>> kill(getpid(), SIGTERM);
>> return (void *) EXIT_FAILURE;
>>
>
> While the patch looks correct, we have a lot of duplication. Simpler
> might be a solution with only one exit label altogether:
>
Thanks for your review, I will modify the patch and send patch v2 according to
your suggestion.
BTW, do I need to split this patch into two patches, one to solve the memleak
and the other to optimizes the redundant code?
Thanks,
Alex
> diff --git i/qemu-nbd.c w/qemu-nbd.c
> index a7075c5419d7..d7bdcd0011ba 100644
> --- i/qemu-nbd.c
> +++ w/qemu-nbd.c
> @@ -265,8 +265,8 @@ static void *nbd_client_thread(void *arg)
> char *device = arg;
> NBDExportInfo info = { .request_sizes = false, .name = g_strdup("") };
> QIOChannelSocket *sioc;
> -int fd;
> -int ret;
> +int fd = -1;
> +int ret = EXIT_FAILURE;
> pthread_t show_parts_thread;
> Error *local_error = NULL;
>
> @@ -278,26 +278,24 @@ static void *nbd_client_thread(void *arg)
> goto out;
> }
>
> -ret = nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
> -NULL, NULL, NULL, , _error);
> -if (ret < 0) {
> +if (nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
> + NULL, NULL, NULL, , _error) < 0) {
> if (local_error) {
> error_report_err(local_error);
> }
> -goto out_socket;
> +goto out;
> }
>
> fd = open(device, O_RDWR);
> if (fd < 0) {
> /* Linux-only, we can use %m in printf. */
> error_report("Failed to open %s: %m", device);
> -goto out_socket;
> +goto out;
> }
>
> -ret = nbd_init(fd, sioc, , _error);
> -if (ret < 0) {
> +if (nbd_init(fd, sioc, , _error) < 0) {
> error_report_err(local_error);
> -goto out_fd;
> +goto out;
> }
>
> /* update partition table */
> @@ -311,24 +309,18 @@ static void *nbd_client_thread(void *arg)
> dup2(STDOUT_FILENO, STDERR_FILENO);
> }
>
> -ret = nbd_client(fd);
> -if (ret) {
> -goto out_fd;
> +if (nbd_client(fd) == 0) {
> +ret = EXIT_SUCCESS;
> }
> -close(fd);
> -object_unref(OBJECT(sioc));
> -g_free(info.name);
> -kill(getpid(), SIGTERM);
> -return (void *) EXIT_SUCCESS;
>
> -out_fd:
> -close(fd);
> -out_socket:
> + out:
> +if (fd >= 0) {
> +close(fd);
> +}
> object_unref(OBJECT(sioc));
> -out:
> g_free(info.name);
> kill(getpid(), SIGTERM);
> -return (void *) EXIT_FAILURE;
> +return (void *) (intptr_t) ret;
> }
> #endif /* HAVE_NBD_DEVICE */
>
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Tue, Dec 01, 2020 at 10:23:57PM +0100, Paolo Bonzini wrote: > On 01/12/20 20:35, Kevin Wolf wrote: > > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > > I don't think this is actually a new things. We already have types and > > commands declared with things like 'if': 'defined(TARGET_S390X)'. > > As far as I understand, QAPI generated files are already built per > > target, so not compiling things into all binaries should be entirely > > possible. > > There is some complication due to having different discriminators per > target. So yes it should be possible. But probably best left after objects > because it's so much bigger a task and because objects have a bit more > freedom for experimentation (less ties to other qdev-specific concepts, e.g. > the magic "bus" property). > > > So maybe only the abstract base class that actually defines the machine > > properties (like generic-pc-machine) should be described in QAPI, and > > then the concrete machine types would inherit from it without being > > described in QAPI themselves? > > Yes, maybe. > > > > 1) whether to generate _all_ boilerplate or only properties > > > > I would like to generate as much boilerplate as possible. That is, I > > don't want to constrain us to only properties, but at the same time, I'm > > not sure if it's possible to get rid of all boilerplate. > > > > Basically, the vision I have in mind is that QAPI would generate code > > that is the same for most instances, and then provide an option that > > prevents code generation for a specific part for more complicated cases, > > so that the respective function can (and must) be provided in the C > > source. > > Ok, so that's a bit different from what I am thinking of. I don't care very > much about the internal boilerplate, only the external interface for > configuration. So I don't care about type registration, dynamic cast macros > etc., only essentially the part that leads to ucc->complete. > > > > 2) whether we want to introduce a separation between configuration > > > schema and run-time state > > > > You mean the internal run-time state? How is this separation not already > > present with getter/setter functions for each property? In many cases > > they just directly access the run-time state, but there are other cases > > where they actually do things. > > I mean moving more towards the blockdev/chardev way of doing things, > increasing the separation very much by having separate configuration structs > that have (potentially) no link to the run-time state struct. > > > > 3) in the latter case, whether properties will survive at all---iothread > > > and > > > throttle-groups don't really need them even if they're writable after > > > creation. > > > > How do you define properties, i.e. at which point would they stop > > existing and what would be a non-property alternative? > > Properties are only a useful concept if they have a use. If > -object/object_add/object-add can do the same job without properties, > properties are not needed anymore. Do you mean "not needed for -object anymore"? Properties are still used by internal C code (esp. board code), -device/device_add, -machine, -cpu, and debugging commands (like "info qtree" and qom-list/qom-get/qom-set). > > Right now QOM is all about exposing properties, and having multiple > interfaces to set them (by picking a different visitor). But in practice > most QOM objects have a lifetime that consists of 1) set properties 2) flip > a switch (realized/complete/open) 3) let the object live on its own. 1+2 > are a single monitor command or CLI option; during 3 you access the object > through monitor commands, not properties. I agree with this, except for the word "all" in "QOM is all about". QOM is also an extensively used internal QEMU API, including internal usage of the QOM property system. > > > So in summary, it seems to me that the QOM way is more flexible because > > you can get both models out of it. Whether we actually need this > > flexibility I can't say. > > I'm thinking there's no need for it, but maybe I'm overly optimistic. > > > * Configuration options are described in the QAPI schema. This is mainly > >for object creation, but runtime modifiable properties are a subset of > >this. > > > > * Properties are generated for each option. By default, the getter > >just returns the value from the configuration at creation time, though > >generation of it can be disabled so that it can be overridden. Also, > >setters just return an error by default. > > > > * Property setters aren't called for object creation. Instead, the > >relevant ObjectOptions branch is made available to some init method. > > > > * Runtime modifiable properties (declared as such in the schema) don't > >get the default setter, so you have to provide an implementation for > >them. > > I wouldn't bother with properties at all in the QAPI schema. Just do the > first and third
Re: [PATCH v3 00/17] 64bit block-layer
On 12/1/20 10:07 AM, Vladimir Sementsov-Ogievskiy wrote: > Hi! > > I'm sorry, I should have pinged it, or resend, or suggest to pull at > least a half long ago :( > > I've rebased it on master and make some fixes. > > What to do next? I can just resend. But I'm afraid that Eric's careful > audits may be out of date: time passed, there is no guarantee that > callers are not changed. Really sorry :( > So r-b marks are not applicable as well, yes? If you think the rebase has fundamentally changed things, then dropping the r-b is safest. I will probably spend a good time on the audit again, but this time, I want to see the project through to completion, and am willing to take patches through my NBD tree if Kevin or other block maintainers do not have enough time to take it through a broader block tree. I can justify it because I have a specific patch in NBD that will benefit from this audit - I want to rever 890cbccb08 in favor of using saner 64-bit APIs throughout the block layer. But I am also aware that your patches touch more than NBD, so even if Kevin can't commit to a full review, I will at least try to get his Acked-by. > > But if I just resend it with no r-bs, is it feasible to review/merge it > in a finite time? So that audits of patches will not become outdated? Yes, let's agree to put a lot more effort into getting this series in for 6.0. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 01/12/20 20:35, Kevin Wolf wrote: Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: I don't think this is actually a new things. We already have types and commands declared with things like 'if': 'defined(TARGET_S390X)'. As far as I understand, QAPI generated files are already built per target, so not compiling things into all binaries should be entirely possible. There is some complication due to having different discriminators per target. So yes it should be possible. But probably best left after objects because it's so much bigger a task and because objects have a bit more freedom for experimentation (less ties to other qdev-specific concepts, e.g. the magic "bus" property). So maybe only the abstract base class that actually defines the machine properties (like generic-pc-machine) should be described in QAPI, and then the concrete machine types would inherit from it without being described in QAPI themselves? Yes, maybe. 1) whether to generate _all_ boilerplate or only properties I would like to generate as much boilerplate as possible. That is, I don't want to constrain us to only properties, but at the same time, I'm not sure if it's possible to get rid of all boilerplate. Basically, the vision I have in mind is that QAPI would generate code that is the same for most instances, and then provide an option that prevents code generation for a specific part for more complicated cases, so that the respective function can (and must) be provided in the C source. Ok, so that's a bit different from what I am thinking of. I don't care very much about the internal boilerplate, only the external interface for configuration. So I don't care about type registration, dynamic cast macros etc., only essentially the part that leads to ucc->complete. 2) whether we want to introduce a separation between configuration schema and run-time state You mean the internal run-time state? How is this separation not already present with getter/setter functions for each property? In many cases they just directly access the run-time state, but there are other cases where they actually do things. I mean moving more towards the blockdev/chardev way of doing things, increasing the separation very much by having separate configuration structs that have (potentially) no link to the run-time state struct. 3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation. How do you define properties, i.e. at which point would they stop existing and what would be a non-property alternative? Properties are only a useful concept if they have a use. If -object/object_add/object-add can do the same job without properties, properties are not needed anymore. Right now QOM is all about exposing properties, and having multiple interfaces to set them (by picking a different visitor). But in practice most QOM objects have a lifetime that consists of 1) set properties 2) flip a switch (realized/complete/open) 3) let the object live on its own. 1+2 are a single monitor command or CLI option; during 3 you access the object through monitor commands, not properties. So in summary, it seems to me that the QOM way is more flexible because you can get both models out of it. Whether we actually need this flexibility I can't say. I'm thinking there's no need for it, but maybe I'm overly optimistic. * Configuration options are described in the QAPI schema. This is mainly for object creation, but runtime modifiable properties are a subset of this. * Properties are generated for each option. By default, the getter just returns the value from the configuration at creation time, though generation of it can be disabled so that it can be overridden. Also, setters just return an error by default. * Property setters aren't called for object creation. Instead, the relevant ObjectOptions branch is made available to some init method. * Runtime modifiable properties (declared as such in the schema) don't get the default setter, so you have to provide an implementation for them. I wouldn't bother with properties at all in the QAPI schema. Just do the first and third bullet. Declaring read-only QOM properties is trivial. So while this series is doing only one part of the whole solution, that the second part is missing doesn't make the first part wrong. Yeah, I think it's clear that for the long term we're not really disagreeing (or perhaps I'm even more radical than you :)). I'm just worried about having yet another incomplete transition. One possibly nasty detail to consider there is that we sometimes declare the USER_CREATABLE interface in the base class, so ucc->complete is for the base class rather than the actually instantiated class. If we only instantiate leaf classes (I think this is true), we can move USER_CREATABLE there. You can also use a while loop covering
Re: [PATCH v11 1/7] Introduce yank feature
On 12/1/20 2:43 PM, Eric Blake wrote:
> On 11/15/20 5:36 AM, Lukas Straub wrote:
>> The yank feature allows to recover from hanging qemu by "yanking"
>
> "allows to $verb" is not idiomatic English, better is "allows $subject
> to verb" or "allows ${verb}ing". In this case, I suggest "The yank
> feature allows the recovery of a hung qemu by "yanking" at various parts".
>
>> at various parts. Other qemu systems can register themselves and
>> multiple yank functions. Then all yank functions for selected
>> instances can be called by the 'yank' out-of-band qmp command.
>> Available instances can be queried by a 'query-yank' oob command.
>>
>> Signed-off-by: Lukas Straub
>> Acked-by: Stefan Hajnoczi
>> Reviewed-by: Markus Armbruster
>> ---
>
>> +# @YankInstanceType:
>> +#
>> +# An enumeration of yank instance types. See @YankInstance for more
>> +# information.
>> +#
>> +# Since: 6.0
>> +##
>> +{ 'enum': 'YankInstanceType',
>> + 'data': [ 'block-node', 'chardev', 'migration' ] }
>> +
>
>> +##
>> +# @YankInstance:
>> +#
>> +# A yank instance can be yanked with the @yank qmp command to recover from a
>> +# hanging QEMU.
>> +#
>> +# Currently implemented yank instances:
>> +# - nbd block device:
>> +#Yanking it will shut down the connection to the nbd server without
>> +#attempting to reconnect.
>
> Mismatch in documentation; I presume it gets cleaned up later in the
> series, in which case I can live with this patch as-is.
Oh, I see. 'block-node' refers to a block node being served over NBD.
So if you have multiple NBD devices, you choose which one or more nodes
are yanked, rather than blindly yanking all of them at once. I just
found it odd that we mention 'nbd' here but not in the enum; on the
other hand, we may have more than just NBD networked devices where other
types of network-enabled block nodes (ceph, sheepdog, gluster...) might
also add yank support.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v11 2/7] block/nbd.c: Add yank feature
On 11/15/20 5:36 AM, Lukas Straub wrote:
> Register a yank function which shuts down the socket and sets
> s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
> error occured.
occurred
>
> Signed-off-by: Lukas Straub
> Acked-by: Stefan Hajnoczi
> ---
> block/nbd.c | 154 +++-
> 1 file changed, 93 insertions(+), 61 deletions(-)
>
> @@ -166,12 +168,12 @@ static void nbd_clear_bdrvstate(BDRVNBDState *s)
> static void nbd_channel_error(BDRVNBDState *s, int ret)
> {
> if (ret == -EIO) {
> -if (s->state == NBD_CLIENT_CONNECTED) {
> +if (qatomic_load_acquire(>state) == NBD_CLIENT_CONNECTED) {
This may have interesting interactions with Vladimir's latest patches to
make NBD connection re-startable, but we'll sort that out as needed.
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg07012.html
The patch seems big; I might have broken it into two pieces (conversion
of existing logic to use qatomic_*() accesses instead of direct s->state
manipulation, and then adding new logic). But I'm not going to hold up
the series demanding for a split at this time.
> @@ -424,12 +427,12 @@ static void *connect_thread_func(void *opaque)
> return NULL;
> }
>
> -static QIOChannelSocket *coroutine_fn
> +static int coroutine_fn
> nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
> {
> +int ret;
> QemuThread thread;
> BDRVNBDState *s = bs->opaque;
> -QIOChannelSocket *res;
> NBDConnectThread *thr = s->connect_thread;
>
> qemu_mutex_lock(>mutex);
> @@ -446,10 +449,12 @@ nbd_co_establish_connection(BlockDriverState *bs, Error
> **errp)
> case CONNECT_THREAD_SUCCESS:
> /* Previous attempt finally succeeded in background */
> thr->state = CONNECT_THREAD_NONE;
> -res = thr->sioc;
> +s->sioc = thr->sioc;
> thr->sioc = NULL;
> +yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
> + nbd_yank, bs);
> qemu_mutex_unlock(>mutex);
> -return res;
> +return 0;
Looks sensible.
> @@ -1745,6 +1759,15 @@ static int nbd_client_reopen_prepare(BDRVReopenState
> *state,
> return 0;
> }
>
> +static void nbd_yank(void *opaque)
> +{
> +BlockDriverState *bs = opaque;
> +BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> +
> +qatomic_store_release(>state, NBD_CLIENT_QUIT);
> +qio_channel_shutdown(QIO_CHANNEL(s->sioc), QIO_CHANNEL_SHUTDOWN_BOTH,
> NULL);
> +}
> +
Yep, that does indeed tell qemu to give up on the NBD socket right away.
Reviewed-by: Eric Blake
And sorry it's taken me so long to actually stare at this series.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v11 1/7] Introduce yank feature
On 11/15/20 5:36 AM, Lukas Straub wrote:
> The yank feature allows to recover from hanging qemu by "yanking"
"allows to $verb" is not idiomatic English, better is "allows $subject
to verb" or "allows ${verb}ing". In this case, I suggest "The yank
feature allows the recovery of a hung qemu by "yanking" at various parts".
> at various parts. Other qemu systems can register themselves and
> multiple yank functions. Then all yank functions for selected
> instances can be called by the 'yank' out-of-band qmp command.
> Available instances can be queried by a 'query-yank' oob command.
>
> Signed-off-by: Lukas Straub
> Acked-by: Stefan Hajnoczi
> Reviewed-by: Markus Armbruster
> ---
> +# @YankInstanceType:
> +#
> +# An enumeration of yank instance types. See @YankInstance for more
> +# information.
> +#
> +# Since: 6.0
> +##
> +{ 'enum': 'YankInstanceType',
> + 'data': [ 'block-node', 'chardev', 'migration' ] }
> +
> +##
> +# @YankInstance:
> +#
> +# A yank instance can be yanked with the @yank qmp command to recover from a
> +# hanging QEMU.
> +#
> +# Currently implemented yank instances:
> +# - nbd block device:
> +#Yanking it will shut down the connection to the nbd server without
> +#attempting to reconnect.
Mismatch in documentation; I presume it gets cleaned up later in the
series, in which case I can live with this patch as-is.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH] qemu-nbd: Fix a memleak in nbd_client_thread()
On 12/1/20 12:13 AM, Alex Chen wrote:
> When the qio_channel_socket_connect_sync() fails
> we should goto 'out_socket' label to free the 'sioc' instead of
> goto 'out' label.
> In addition, now the 'out' label is useless, delete it.
>
> Reported-by: Euler Robot
> Signed-off-by: Alex Chen
> ---
> qemu-nbd.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 47587a709e..643b0777c0 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -275,7 +275,7 @@ static void *nbd_client_thread(void *arg)
> saddr,
> _error) < 0) {
> error_report_err(local_error);
> -goto out;
> +goto out_socket;
> }
>
> ret = nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
> @@ -325,7 +325,6 @@ out_fd:
> close(fd);
> out_socket:
> object_unref(OBJECT(sioc));
> -out:
> g_free(info.name);
> kill(getpid(), SIGTERM);
> return (void *) EXIT_FAILURE;
>
While the patch looks correct, we have a lot of duplication. Simpler
might be a solution with only one exit label altogether:
diff --git i/qemu-nbd.c w/qemu-nbd.c
index a7075c5419d7..d7bdcd0011ba 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -265,8 +265,8 @@ static void *nbd_client_thread(void *arg)
char *device = arg;
NBDExportInfo info = { .request_sizes = false, .name = g_strdup("") };
QIOChannelSocket *sioc;
-int fd;
-int ret;
+int fd = -1;
+int ret = EXIT_FAILURE;
pthread_t show_parts_thread;
Error *local_error = NULL;
@@ -278,26 +278,24 @@ static void *nbd_client_thread(void *arg)
goto out;
}
-ret = nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
-NULL, NULL, NULL, , _error);
-if (ret < 0) {
+if (nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
+ NULL, NULL, NULL, , _error) < 0) {
if (local_error) {
error_report_err(local_error);
}
-goto out_socket;
+goto out;
}
fd = open(device, O_RDWR);
if (fd < 0) {
/* Linux-only, we can use %m in printf. */
error_report("Failed to open %s: %m", device);
-goto out_socket;
+goto out;
}
-ret = nbd_init(fd, sioc, , _error);
-if (ret < 0) {
+if (nbd_init(fd, sioc, , _error) < 0) {
error_report_err(local_error);
-goto out_fd;
+goto out;
}
/* update partition table */
@@ -311,24 +309,18 @@ static void *nbd_client_thread(void *arg)
dup2(STDOUT_FILENO, STDERR_FILENO);
}
-ret = nbd_client(fd);
-if (ret) {
-goto out_fd;
+if (nbd_client(fd) == 0) {
+ret = EXIT_SUCCESS;
}
-close(fd);
-object_unref(OBJECT(sioc));
-g_free(info.name);
-kill(getpid(), SIGTERM);
-return (void *) EXIT_SUCCESS;
-out_fd:
-close(fd);
-out_socket:
+ out:
+if (fd >= 0) {
+close(fd);
+}
object_unref(OBJECT(sioc));
-out:
g_free(info.name);
kill(getpid(), SIGTERM);
-return (void *) EXIT_FAILURE;
+return (void *) (intptr_t) ret;
}
#endif /* HAVE_NBD_DEVICE */
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > On 01/12/20 17:20, Kevin Wolf wrote: > > Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: > > > For devices it's just the practical issue that there are too many to have > > > something like this series. For machine types the main issue is that the > > > version-specific machine types would have to be defined in one more place. > > > > Sure, the number of devices means that we can't convert all of them at > > once. And we don't need to, we can make the change incrementally. > > There's also the question that most devices are not present in all binaries. > So QAPI introspection would tell you what properties are supported but not > what devices are. Also the marshaling/unmarshaling code for hundreds of > devices would bloat the QEMU executables unnecessarily (it'd all be > reachable from visit_DeviceOptions so it'd not be possible to compile it > out, even with LTO). I don't think this is actually a new things. We already have types and commands declared with things like 'if': 'defined(TARGET_S390X)'. As far as I understand, QAPI generated files are already built per target, so not compiling things into all binaries should be entirely possible. What probably needs a solution is that an explicit 'if' in the schema would duplicate information that is actually defined in the build system configuration. So we would somehow need a common source for both. > Plus the above issue with machine types. As I said, I don't know the machine type code well, so I'm not really sure about the best way there. But maybe QAPI isn't for version-specific machine types. I think they never have additional properties, but only different init functions. So their description in QAPI would be essentially empty anyway. So maybe only the abstract base class that actually defines the machine properties (like generic-pc-machine) should be described in QAPI, and then the concrete machine types would inherit from it without being described in QAPI themselves? > > > > > The single struct doesn't bother me _too much_ actually. What > > > > > bothers me is > > > > > that it won't be a single source of all QOM objects, only those that > > > > > happen > > > > > to be created by object-add. > > > > > > > > But isn't it only natural that a list of these objects will exist in > > > > some way, implicitly or explicitly? object-add must somehow decide which > > > > object types it allows the user to create. > > > > > > There's already one, it's objects that implement user creatable. I don't > > > think having it as a QAPI enum (or discriminator) is worthwhile, and it's > > > one more thing that can go out of sync between the QAPI schema and the C > > > code. > > > > Well, we all know that this series duplicates things. But at the same > > time, we also know that this isn't going to be the final state. > > > > Once QAPI learns about QOM inheritance (which it has to if it should > > generate the boilerplate), it will know which objects are user creatable > > without a an explicitly defined separate enum. > > > > I think it will still need to have the enum internally, but as long as > > it's automatically generated, that shouldn't be a big deal. > > Right, I don't want to have the final state now but I'd like to have a > rough idea of the plan: > > 1) whether to generate _all_ boilerplate or only properties I would like to generate as much boilerplate as possible. That is, I don't want to constrain us to only properties, but at the same time, I'm not sure if it's possible to get rid of all boilerplate. Basically, the vision I have in mind is that QAPI would generate code that is the same for most instances, and then provide an option that prevents code generation for a specific part for more complicated cases, so that the respective function can (and must) be provided in the C source. > 2) whether we want to introduce a separation between configuration > schema and run-time state You mean the internal run-time state? How is this separation not already present with getter/setter functions for each property? In many cases they just directly access the run-time state, but there are other cases where they actually do things. Or do you mean between create-time options and properties that can be written at runtime? In that case, yes, I think that would be very desirable because mashing them together has resulted in lots of bugs. > 3) in the latter case, whether properties will survive at all---iothread and > throttle-groups don't really need them even if they're writable after > creation. How do you define properties, i.e. at which point would they stop existing and what would be a non-property alternative? Outside of QOM, usually have a query-* command that returns the whole state rather than a single property, and possibly individual QMP commands to update the state. blockdev-reopen takes a new configuration (the same structure as blockdev-add) and then applies
[RFC PATCH v2 4/4] hw/scsi/megasas: Have incorrect cdb return MFI_STAT_ABORT_NOT_POSSIBLE
Avoid out-of-bound array access with invalid CDB is provided.
Signed-off-by: Philippe Mathieu-Daudé
---
RFC because no clue how hardware works
---
hw/scsi/megasas.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index f5ad4425b5b..7e7cbb8854b 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1680,7 +1680,15 @@ static int megasas_handle_scsi(MegasasState *s,
MegasasCmd *cmd,
if (cdb_len > 0) {
len = scsi_cdb_length(cdb);
}
-assert(len > 0 && cdb_len >= len);
+if (len < 0 || len < cdb_len) {
+trace_megasas_scsi_invalid_cdb_len(mfi_frame_desc(frame_cmd),
+ is_logical, target_id,
+ lun_id, cdb_len);
+megasas_write_sense(cmd, SENSE_CODE(INVALID_FIELD));
+cmd->frame->header.scsi_status = TASK_ABORTED;
+s->event_count++;
+return MFI_STAT_ABORT_NOT_POSSIBLE;
+}
if (is_logical) {
if (target_id >= MFI_MAX_LD || lun_id != 0) {
trace_megasas_scsi_target_not_present(
--
2.26.2
Re: [PATCH v2 3/4] tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer
On 12/1/20 8:10 PM, Philippe Mathieu-Daudé wrote: > Add a reproducer which triggers (without the previous patch): > > $ make check-qtest-x86_64 > Running test qtest-x86_64/fuzz-test > qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion > `cdb_len > 0 && scsi_cdb_length(cdb) <= cdb_len' failed. > tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6 > (Aborted) (core dumped) > ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0) > > Signed-off-by: Alexander Bulekov > Signed-off-by: Philippe Mathieu-Daudé > --- > tests/qtest/fuzz-test.c | 19 +++ > 1 file changed, 19 insertions(+) Oops this should be patch #4...
[PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
cdb_len can not be zero... (or less than 6) here, else we have a
out-of-bound read first in scsi_cdb_length():
71 int scsi_cdb_length(uint8_t *buf)
72 {
73 int cdb_len;
74
75 switch (buf[0] >> 5) {
76 case 0:
77 cdb_len = 6;
78 break;
Then another out-of-bound read when the size returned by
scsi_cdb_length() is used.
Figured out after reviewing:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
And reproduced fuzzing:
qemu-fuzz-i386: hw/scsi/megasas.c:1679: int megasas_handle_scsi(MegasasState
*, MegasasCmd *, int):
Assertion `len > 0 && cdb_len >= len' failed.
==1689590== ERROR: libFuzzer: deadly signal
#8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
#9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
#10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
#11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
#12 0x55a1b981972e in memory_region_write_accessor softmmu/memory.c:491:5
#13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
#14 0x55a1b981972e in memory_region_dispatch_write
softmmu/memory.c:1501:16
#15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
#16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
#17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
#18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
#19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
Inspired-by: Daniele Buono
Inspired-by: Alexander Bulekov
Signed-off-by: Philippe Mathieu-Daudé
---
hw/scsi/megasas.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 1a5fc5857db..f5ad4425b5b 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s,
MegasasCmd *cmd,
{
uint8_t *cdb;
int target_id, lun_id, cdb_len;
+int len = -1;
bool is_write;
struct SCSIDevice *sdev = NULL;
bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
@@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s,
MegasasCmd *cmd,
lun_id = cmd->frame->header.lun_id;
cdb_len = cmd->frame->header.cdb_len;
+if (cdb_len > 0) {
+len = scsi_cdb_length(cdb);
+}
+assert(len > 0 && cdb_len >= len);
if (is_logical) {
if (target_id >= MFI_MAX_LD || lun_id != 0) {
trace_megasas_scsi_target_not_present(
--
2.26.2
[PATCH v2 1/4] tests/qtest/fuzz-test: Quit test_lp1878642 once done
Missed in fd250172842 ("qtest: add a reproducer for LP#1878642").
Reviewed-by: Thomas Huth
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/fuzz-test.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 9cb4c42bdea..87b72307a5b 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -45,6 +45,7 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_outl(s, 0xcf8, 0x8400f841);
qtest_outl(s, 0xcfc, 0xebed205d);
qtest_outl(s, 0x5d02, 0xebed205d);
+qtest_quit(s);
}
int main(int argc, char **argv)
--
2.26.2
[PATCH v2 3/4] tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer
Add a reproducer which triggers (without the previous patch):
$ make check-qtest-x86_64
Running test qtest-x86_64/fuzz-test
qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
`cdb_len > 0 && scsi_cdb_length(cdb) <= cdb_len' failed.
tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
(Aborted) (core dumped)
ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
Signed-off-by: Alexander Bulekov
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/fuzz-test.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 87b72307a5b..31f90cfb4fc 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -48,6 +48,23 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_quit(s);
}
+static void test_megasas_cdb_len_zero(void)
+{
+QTestState *s;
+
+s = qtest_init("-M pc -nodefaults "
+ "-device megasas-gen2 -device scsi-cd,drive=null0 "
+ "-blockdev driver=null-co,read-zeroes=on,node-name=null0");
+
+qtest_outl(s, 0xcf8, 0x80001011);
+qtest_outb(s, 0xcfc, 0xbb);
+qtest_outl(s, 0xcf8, 0x80001002);
+qtest_outl(s, 0xcfc, 0xf3ff2966);
+qtest_writeb(s, 0x4600, 0x03);
+qtest_outw(s, 0xbb40, 0x460b);
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -59,6 +76,8 @@ int main(int argc, char **argv)
test_lp1878263_megasas_zero_iov_cnt);
qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
test_lp1878642_pci_bus_get_irq_level_assert);
+qtest_add_func("fuzz/test_megasas_cdb_len_zero",
+ test_megasas_cdb_len_zero);
}
return g_test_run();
--
2.26.2
[PATCH v2 0/4] hw/scsi/megasas: Avoid buffer overrun in megasas_handle_scsi()
FWIW megasas is not use by KVM. Not sure what is the proper fix, but at least we have a reproducer. Since v1: - Fix assert() condition - Extract reproducer in different patch for git-bisect (thuth) - Add simpler reproducer from Alex - Try better scsi error Philippe Mathieu-Daudé (4): tests/qtest/fuzz-test: Quit test_lp1878642 once done hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi() tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer hw/scsi/megasas: Have incorrect cdb return MFI_STAT_ABORT_NOT_POSSIBLE hw/scsi/megasas.c | 13 + tests/qtest/fuzz-test.c | 20 2 files changed, 33 insertions(+) -- 2.26.2
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On Tue, Dec 01, 2020 at 06:16:14PM +0100, Paolo Bonzini wrote: > On 01/12/20 17:20, Kevin Wolf wrote: [...] > > BlockdevOptions is about external interfaces, not about > > implementation details. Same thing as QOM properties are external > > interfaces, not implementation details. There may be fields in the > > internal state that correspond 1:1 to the externally visible QAPI > > type/QOM property, but it's not necessarily the case. > > Right. It may well be that we decide that, as a result of this series, > QOM's property interface is declared essentially a failed experiment. I > wouldn't be surprised, and that's why I want to understand better where we > want to go. > > For example, Eduardo is focusing specifically on external interfaces that > correspond 1:1 to the internal implementation. If we decide that > non-1:1-mappings and checks on mandatory properties are an important part of > QOM, that may make large parts of his work moot. [...] Whatever we decide, my first and biggest worry is to have a reasonable migration path for any new API. We could describe in detail what's the API we _want_, but we also need a reasonable migration path for the code we already _have_. If a new API that requires manual conversion of existing devices, it will probably coexist with the existing qdev property API for years. (Note that I haven't read this whole thread yet.) >[...] If we decide that most QOM > objects need no properties at all, then we don't want to move more > qdev-specific stuff from to QOM. I don't understand what "move more qdev-specific stuff to QOM" means. I consider the QOM field property API valuable even if we decide the only user of the API will be legacy qdev code, because it separates the core mechanism (that's code that already existed) from qdev-specific policies. > > > QAPI is already here and it's going to stay. QOM doesn't have to > > duplicate input validation that existing code can already perform. > > > > I'm not sure which complexity you think I'm introducing: QAPI is already > > there. I'm adding the schema, which you agree is valuable documentation, > > so we want to have it either case. The actual change to QOM that we have > > in this series is this: > > The complexity is that properties used to be split in two places, and now > they're split in three places. > > It may very well be that this is a good first step to at least have classes > described in the QAPI schema. But since _in the short term_ there are > things that the series makes worse (and has a risk of bringing things out of > sync), I'd like to understand the long term plan and ensure that the QAPI > maintainers are on board with it. > > Can you at least add a comment to all UserCreatable classes that says "if > you add a property, remember to modify ... as well in the QAPI schema"? > > > > Are there any validation bugs that you're fixing? Is that > > > something that cannot be fixed elsewhere, or are you papering over bad QOM > > > coding? (Again, I'm not debating that QOM properties are hard to write > > > correctly). > > > > Yes, I found bugs that the QAPI schema would have prevented. They were > > generally about not checking whether mandatory propertes are actually > > set. > > Okay, I found your series at > https://patchew.org/QEMU/20201130105615.21799-1-kw...@redhat.com/ too, good > to know. > > So that's another useful thing that can be chalked to this series at least > if -object and object_add are converted (and also, another thing against QOM > properties and 1:1 mappings between configuration schema and run-time > state). > > Paolo > -- Eduardo
[PATCH] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
From: Philippe Mathieu-Daudé
cdb_len can not be zero... (or less than 6) here, else we have a
out-of-bound read first in scsi_cdb_length():
71 int scsi_cdb_length(uint8_t *buf)
72 {
73 int cdb_len;
74
75 switch (buf[0] >> 5) {
76 case 0:
77 cdb_len = 6;
78 break;
Then another out-of-bound read when the size returned by
scsi_cdb_length() is used.
Add a reproducer which triggers:
$ make check-qtest-x86_64
Running test qtest-x86_64/fuzz-test
qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
`cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len' failed.
tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
(Aborted) (core dumped)
ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
Signed-off-by: Alexander Bulekov
Signed-off-by: Philippe Mathieu-Daudé
---
Ran it through the minimizer - No more blobs.
hw/scsi/megasas.c | 1 +
tests/qtest/fuzz-test.c | 19 +++
2 files changed, 20 insertions(+)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 1a5fc5857d..28efd09411 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1676,6 +1676,7 @@ static int megasas_handle_scsi(MegasasState *s,
MegasasCmd *cmd,
lun_id = cmd->frame->header.lun_id;
cdb_len = cmd->frame->header.cdb_len;
+assert(cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len);
if (is_logical) {
if (target_id >= MFI_MAX_LD || lun_id != 0) {
trace_megasas_scsi_target_not_present(
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 87b72307a5..31f90cfb4f 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -48,6 +48,23 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_quit(s);
}
+static void test_megasas_cdb_len_zero(void)
+{
+QTestState *s;
+
+s = qtest_init("-M pc -nodefaults "
+ "-device megasas-gen2 -device scsi-cd,drive=null0 "
+ "-blockdev driver=null-co,read-zeroes=on,node-name=null0");
+
+qtest_outl(s, 0xcf8, 0x80001011);
+qtest_outb(s, 0xcfc, 0xbb);
+qtest_outl(s, 0xcf8, 0x80001002);
+qtest_outl(s, 0xcfc, 0xf3ff2966);
+qtest_writeb(s, 0x4600, 0x03);
+qtest_outw(s, 0xbb40, 0x460b);
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -59,6 +76,8 @@ int main(int argc, char **argv)
test_lp1878263_megasas_zero_iov_cnt);
qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
test_lp1878642_pci_bus_get_irq_level_assert);
+qtest_add_func("fuzz/test_megasas_cdb_len_zero",
+ test_megasas_cdb_len_zero);
}
return g_test_run();
--
2.28.0
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
On 01/12/20 17:20, Kevin Wolf wrote: Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: For devices it's just the practical issue that there are too many to have something like this series. For machine types the main issue is that the version-specific machine types would have to be defined in one more place. Sure, the number of devices means that we can't convert all of them at once. And we don't need to, we can make the change incrementally. There's also the question that most devices are not present in all binaries. So QAPI introspection would tell you what properties are supported but not what devices are. Also the marshaling/unmarshaling code for hundreds of devices would bloat the QEMU executables unnecessarily (it'd all be reachable from visit_DeviceOptions so it'd not be possible to compile it out, even with LTO). Plus the above issue with machine types. The single struct doesn't bother me _too much_ actually. What bothers me is that it won't be a single source of all QOM objects, only those that happen to be created by object-add. But isn't it only natural that a list of these objects will exist in some way, implicitly or explicitly? object-add must somehow decide which object types it allows the user to create. There's already one, it's objects that implement user creatable. I don't think having it as a QAPI enum (or discriminator) is worthwhile, and it's one more thing that can go out of sync between the QAPI schema and the C code. Well, we all know that this series duplicates things. But at the same time, we also know that this isn't going to be the final state. Once QAPI learns about QOM inheritance (which it has to if it should generate the boilerplate), it will know which objects are user creatable without a an explicitly defined separate enum. I think it will still need to have the enum internally, but as long as it's automatically generated, that shouldn't be a big deal. Right, I don't want to have the final state now but I'd like to have a rough idea of the plan: 1) whether to generate _all_ boilerplate or only properties 2) whether we want to introduce a separation between configuration schema and run-time state 3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation. These questions have a substantial effect on how to handle this series. Without answers to this questions I cannot understand the long term and its interaction with other long term plans for QOM. A modified QOM might be the right solution, though. I would like to bring QAPI and QOM together because most of these weaknesses are strengths of QAPI. I agree wholeheartedly. But your series goes to the opposite excess. Eduardo is doing work in QOM to mitigate the issues you point out, and you have to meet in the middle with him. Starting with the user-visible parts focuses on the irrelevant parts. QAPI is first and foremost about user-visible parts, and in fact most of the problems I listed are about external interfaces. Yes, but QAPI is also about interfacing with existing code. Also, QAPI does not generate only structs, it also generate C function prototypes. I'm not sure whether a QOM object's more similar to the struct case (what you do here) or to the QMP command case: - there's a 1:1 correspondence between ObjectOptions cases and ucc->complete implementations - there's a registry of object types just like there's one for QMP commands. So another possibility could be that the QAPI generator essentially generated the user_creatable_add_type function that called out to user-provided functions qom_scsi_pr_helper_complete, qom_throttle_group_complete, etc. The arguments to those functions would be the configuration. That is a very interesting prospect (though one that would require changes to the QAPI code generator). BlockdevOptions is about external interfaces, not about implementation details. Same thing as QOM properties are external interfaces, not implementation details. There may be fields in the internal state that correspond 1:1 to the externally visible QAPI type/QOM property, but it's not necessarily the case. Right. It may well be that we decide that, as a result of this series, QOM's property interface is declared essentially a failed experiment. I wouldn't be surprised, and that's why I want to understand better where we want to go. For example, Eduardo is focusing specifically on external interfaces that correspond 1:1 to the internal implementation. If we decide that non-1:1-mappings and checks on mandatory properties are an important part of QOM, that may make large parts of his work moot. If we decide that most QOM objects need no properties at all, then we don't want to move more qdev-specific stuff from to QOM. QAPI is already here and it's going to stay. QOM doesn't have to duplicate input validation that existing
Re: [PATCH v3 00/17] 64bit block-layer
01.12.2020 19:07, Vladimir Sementsov-Ogievskiy wrote: I have an idea: instead of auditing each function callers, can we just make some good assumptions (like that the whole offset/bytes request being aligned to bs->request_alignement doesn't lay inside [0..INT64_MAX] region), check it once in bdrv_check_bytes_request() and assert in each function we convert to int64_t. s/doesn't// -- Best regards, Vladimir
Re: [PATCH v11 0/7] Introduce 'yank' oob qmp command to recover from hanging qemu
Lukas Straub writes: > Hello Everyone, > So here is v11. > @Eric Blake and @Marc-André Lureau: We still need ACKs for NBD and chardev. Once we have them, I can take the series through my tree.
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: > On 30/11/20 19:10, Kevin Wolf wrote: > > Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben: > > > The main problem is that it wouldn't extend well, if at all, to > > > machines and devices. So those would still not be integrated into the > > > QAPI schema. > > > > What do you think is the biggest difference there? Don't devices work > > the same as user creatable objects in that you first set a bunch of > > properties (which would now be done through QAPI instead) and then call > > the completion/realize method that actually makes use of them? > > For devices it's just the practical issue that there are too many to have > something like this series. For machine types the main issue is that the > version-specific machine types would have to be defined in one more place. Sure, the number of devices means that we can't convert all of them at once. And we don't need to, we can make the change incrementally. The only thing we can't do during this incremental phase is switching device_add from 'gen': false to actually using QAPI types. Doing that would be the very last step in the conversion. > > > The single struct doesn't bother me _too much_ actually. What bothers me > > > is > > > that it won't be a single source of all QOM objects, only those that > > > happen > > > to be created by object-add. > > > > But isn't it only natural that a list of these objects will exist in > > some way, implicitly or explicitly? object-add must somehow decide which > > object types it allows the user to create. > > There's already one, it's objects that implement user creatable. I don't > think having it as a QAPI enum (or discriminator) is worthwhile, and it's > one more thing that can go out of sync between the QAPI schema and the C > code. Well, we all know that this series duplicates things. But at the same time, we also know that this isn't going to be the final state. Once QAPI learns about QOM inheritance (which it has to if it should generate the boilerplate), it will know which objects are user creatable without a an explicitly defined separate enum. I think it will still need to have the enum internally, but as long as it's automatically generated, that shouldn't be a big deal. > > I'm also pretty sure that QOM as it exists now is not the right solution > > for any of them because it has some major shortcomings. It's too easy to > > get things wrong (like the writable properties after creation), its > > introspection is rather weak and separated from the QAPI introspection, > > it doesn't encourage documentation, and it involves quite a bit of > > boilerplate and duplicated code between class implementations. > > > > A modified QOM might be the right solution, though. I would like to > > bring QAPI and QOM together because most of these weaknesses are > > strengths of QAPI. > > I agree wholeheartedly. But your series goes to the opposite excess. > Eduardo is doing work in QOM to mitigate the issues you point out, and you > have to meet in the middle with him. Starting with the user-visible parts > focuses on the irrelevant parts. QAPI is first and foremost about user-visible parts, and in fact most of the problems I listed are about external interfaces. It seems to make a lot of sense to me to start there. > > > Mostly because it's more or less the same issue that you have with > > > BlockdevOptions, with the extra complication that this series only deals > > > with the easy one of the four above categories. > > > > I'm not sure which exact problem with BlockdevOptions you mean. The > > reason why the block layer doesn't use BlockdevOptions everywhere is > > -drive support. > > You don't have a single BlockdevOptions* field in the structs of block/. My > interpretation of this is that BlockdevOptions* is a good schema for > configuring things but not for run-time state. I'm not sure what you mean with run-time state. Yes, BlockdevOptions is about external interfaces, not about implementation details. Same thing as QOM properties are external interfaces, not implementation details. There may be fields in the internal state that correspond 1:1 to the externally visible QAPI type/QOM property, but it's not necessarily the case. > > > > Maybe if we don't want to commit to keeping the ObjectOptions schema, > > > > the part that should wait is object-add and I should do the command line > > > > options first? Then the schema remains an implementation detail for now > > > > that is invisible in introspection. > > > > > > I don't see much benefit in converting _any_ of the three actually. The > > > only good reason I see for QAPIfying this is the documentation, and the > > > promise of deduplicating QOM boilerplate. The latter is only a promise, > > > but > > > documentation alone is a damn good reason and it's enough to get this work > > > into a mergeable shape as soon as possible! > > > > I think having some input validation done
Re: [PATCH v3 00/17] 64bit block-layer
Hi!
I'm sorry, I should have pinged it, or resend, or suggest to pull at least a
half long ago :(
I've rebased it on master and make some fixes.
What to do next? I can just resend. But I'm afraid that Eric's careful audits
may be out of date: time passed, there is no guarantee that callers are not
changed. Really sorry :(
So r-b marks are not applicable as well, yes?
But if I just resend it with no r-bs, is it feasible to review/merge it in a
finite time? So that audits of patches will not become outdated?
Any ideas?
I have an idea: instead of auditing each function callers, can we just make some
good assumptions (like that the whole offset/bytes request being aligned to
bs->request_alignement doesn't lay inside [0..INT64_MAX] region), check it once
in bdrv_check_bytes_request() and assert in each function we convert to int64_t.
Then, if somewhere our assumption is wrong, we'll have a crash and fix the bug.
30.04.2020 14:10, Vladimir Sementsov-Ogievskiy wrote:
Hi all!
We want 64bit write-zeroes, and for this, convert all io functions to
64bit.
We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).
Please refer to initial cover-letter
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
for more info.
v3 is available at
https://src.openvz.org/scm/~vsementsov/qemu.git #tag up-64bit-block-layer-v3
v3: Based on "[PATCH v2 0/9] block/io: safer inc/dec in_flight sections"
Add Eric's r-bs, improve commit message with short reasoning of the whole
thing, and Eric's audits (if you don't like something, I'll change or drop for
next series).
Add "Series:" tag to each patch. Just an idea, if it's inappropriate thing,
I'll drop it.
01: add assertion that bytes > 0
02: fix indentation
06: refactor calculations in bdrv_co_write_req_prepare
09,10: simple rebase conflicts solved
Also, cover more drivers by driver-updating patches and fix int flags
to be BdrvRequestFlags flags.
Based-on: <20200427143907.5710-1-vsement...@virtuozzo.com>
Series: 64bit-block-status
Vladimir Sementsov-Ogievskiy (17):
block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit
bytes
block: use int64_t as bytes type in tracked requests
block/io: use int64_t bytes parameter in bdrv_check_byte_request()
block/io: use int64_t bytes in driver wrappers
block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes()
block/io: support int64_t bytes in bdrv_aligned_pwritev()
block/io: support int64_t bytes in bdrv_co_do_copy_on_readv()
block/io: support int64_t bytes in bdrv_aligned_preadv()
block/io: support int64_t bytes in bdrv_co_p{read,write}v_part()
block/io: support int64_t bytes in read/write wrappers
block/io: use int64_t bytes in copy_range
block/block-backend: convert blk io path to use int64_t parameters
block: use int64_t instead of uint64_t in driver read handlers
block: use int64_t instead of uint64_t in driver write handlers
block: use int64_t instead of uint64_t in copy_range driver handlers
block: use int64_t instead of int in driver write_zeroes handlers
block: use int64_t instead of int in driver discard handlers
include/block/block.h | 17 +++--
include/block/block_int.h | 67 -
include/block/throttle-groups.h | 2 +-
include/sysemu/block-backend.h | 26 +++
block/backup-top.c | 14 ++--
block/blkdebug.c| 12 +--
block/blklogwrites.c| 16 ++--
block/blkreplay.c | 8 +-
block/blkverify.c | 10 +--
block/block-backend.c | 60 +++
block/bochs.c | 4 +-
block/cloop.c | 4 +-
block/commit.c | 2 +-
block/copy-on-read.c| 14 ++--
block/crypto.c | 8 +-
block/curl.c| 3 +-
block/dmg.c | 4 +-
block/file-posix.c | 46
block/file-win32.c | 8 +-
block/filter-compress.c | 15 ++--
block/gluster.c | 14 ++--
block/io.c | 126 +---
block/iscsi.c | 34 ++---
block/mirror.c | 8 +-
block/nbd.c | 18 +++--
block/nfs.c | 12 +--
block/null.c| 18 +++--
block/nvme.c| 38 +++---
block/qcow.c| 16 ++--
block/qcow2.c | 34 +
block/qed.c | 17 -
block/quorum.c | 9 ++-
block/raw-format.c | 36 -
block/rbd.c | 10 ++-
block/sheepdog.c| 11 ++-
block/throttle-groups.c | 5 +-
block/throttle.c| 14 ++--
block/vdi.c
Re: [PATCH 2/3] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
On 01/12/2020 16.13, Philippe Mathieu-Daudé wrote:
> cdb_len can not be zero... (or less than 6) here, else we have a
> out-of-bound read first in scsi_cdb_length():
>
> 71 int scsi_cdb_length(uint8_t *buf)
> 72 {
> 73 int cdb_len;
> 74
> 75 switch (buf[0] >> 5) {
> 76 case 0:
> 77 cdb_len = 6;
> 78 break;
>
> Then another out-of-bound read when the size returned by
> scsi_cdb_length() is used.
>
> Add a reproducer which triggers:
>
> $ make check-qtest-x86_64
> Running test qtest-x86_64/fuzz-test
> qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
> `cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len' failed.
> tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
> (Aborted) (core dumped)
> ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
>
> Inspired-by: Alexander Bulekov
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/scsi/megasas.c | 1 +
> tests/qtest/fuzz-test.c | 196
For the final version, I think you should add the fuzz-test after the fix
(patch 3) ... otherwise this breaks bisection later...
Thomas
Re: [PATCH 1/3] tests/qtest/fuzz-test: Quit test_lp1878642 once done
On 01/12/2020 16.13, Philippe Mathieu-Daudé wrote:
> Missed in fd250172842 ("qtest: add a reproducer for LP#1878642").
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/qtest/fuzz-test.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 9cb4c42bdea..87b72307a5b 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -45,6 +45,7 @@ static void
> test_lp1878642_pci_bus_get_irq_level_assert(void)
> qtest_outl(s, 0xcf8, 0x8400f841);
> qtest_outl(s, 0xcfc, 0xebed205d);
> qtest_outl(s, 0x5d02, 0xebed205d);
> +qtest_quit(s);
> }
>
> int main(int argc, char **argv)
>
Reviewed-by: Thomas Huth
[RFC PATCH 3/3] hw/scsi/megasas: Have incorrect cdb return MFI_STAT_ABORT_NOT_POSSIBLE
Avoid out-of-bound array access with invalid CDB is provided.
Signed-off-by: Philippe Mathieu-Daudé
---
RFC because I have no clue how hardware works.
Maybe returning MFI_STAT_ARRAY_INDEX_INVALID is better?
Do we need to call megasas_write_sense()?
hw/scsi/megasas.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 28efd094111..d89a3c8c3ce 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1676,7 +1676,12 @@ static int megasas_handle_scsi(MegasasState *s,
MegasasCmd *cmd,
lun_id = cmd->frame->header.lun_id;
cdb_len = cmd->frame->header.cdb_len;
-assert(cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len);
+if (!cdb_len || scsi_cdb_length(cdb) < cdb_len) {
+trace_megasas_scsi_invalid_cdb_len(mfi_frame_desc(frame_cmd),
+ is_logical, target_id,
+ lun_id, cdb_len);
+return MFI_STAT_ABORT_NOT_POSSIBLE;
+}
if (is_logical) {
if (target_id >= MFI_MAX_LD || lun_id != 0) {
trace_megasas_scsi_target_not_present(
--
2.26.2
[PATCH 1/3] tests/qtest/fuzz-test: Quit test_lp1878642 once done
Missed in fd250172842 ("qtest: add a reproducer for LP#1878642").
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/fuzz-test.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 9cb4c42bdea..87b72307a5b 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -45,6 +45,7 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_outl(s, 0xcf8, 0x8400f841);
qtest_outl(s, 0xcfc, 0xebed205d);
qtest_outl(s, 0x5d02, 0xebed205d);
+qtest_quit(s);
}
int main(int argc, char **argv)
--
2.26.2
[PATCH 2/3] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
cdb_len can not be zero... (or less than 6) here, else we have a
out-of-bound read first in scsi_cdb_length():
71 int scsi_cdb_length(uint8_t *buf)
72 {
73 int cdb_len;
74
75 switch (buf[0] >> 5) {
76 case 0:
77 cdb_len = 6;
78 break;
Then another out-of-bound read when the size returned by
scsi_cdb_length() is used.
Add a reproducer which triggers:
$ make check-qtest-x86_64
Running test qtest-x86_64/fuzz-test
qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
`cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len' failed.
tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
(Aborted) (core dumped)
ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
Inspired-by: Alexander Bulekov
Signed-off-by: Philippe Mathieu-Daudé
---
hw/scsi/megasas.c | 1 +
tests/qtest/fuzz-test.c | 196
2 files changed, 197 insertions(+)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 1a5fc5857db..28efd094111 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1676,6 +1676,7 @@ static int megasas_handle_scsi(MegasasState *s,
MegasasCmd *cmd,
lun_id = cmd->frame->header.lun_id;
cdb_len = cmd->frame->header.cdb_len;
+assert(cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len);
if (is_logical) {
if (target_id >= MFI_MAX_LD || lun_id != 0) {
trace_megasas_scsi_target_not_present(
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 87b72307a5b..42e88d761b8 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -48,6 +48,200 @@ static void
test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_quit(s);
}
+static void test_megasas_cdb_len_zero(void)
+{
+static const unsigned char megasas_blob1[] = {
+0x03, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60
+},
+megasas_blob2[] = {
+0x03, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e,
+0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa,
+0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x2e, 0x3e, 0x00, 0xff, 0x00,
+0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d,
+0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00,
+0xff, 0xff, 0x59, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe,
+0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a,
+0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x84, 0x3e, 0x00,
+0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51,
+0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00,
+0xeb, 0x00, 0xff, 0xff, 0xaf, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60,
+0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15,
+0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0xda,
+0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00,
+0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14,
+0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x05, 0x3e, 0x00, 0xff, 0x00, 0x00,
+0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea,
+0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff,
+0xff, 0x30, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff,
+0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff,
+0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x5b, 0x3e, 0x00, 0xff,
+0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00,
+0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb,
+0x00, 0xff, 0xff, 0x86, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff,
+0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a,
+0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0xb1, 0x3e,
+0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17,
+0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02,
+0x00, 0xeb, 0x00, 0xff, 0xff, 0xdc, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00,
+0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46,
+0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff,
+0x07, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e,
+0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa,
+0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x32, 0x3e, 0x00, 0xff, 0x00,
+0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d,
+0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00,
+0xff, 0xff, 0x5d, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe,
+0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea,
[PATCH 0/3] hw/scsi/megasas: Avoid buffer overrun in megasas_handle_scsi()
FWIW megasas is not use by KVM. Not sure what is the proper fix, but at least we have a reproducer. We might improve "scsi/utils" by adding length argument to scsi_cdb_length() and check valid there, but this will take time (large refactor). Add assertions there is too violent. Philippe Mathieu-Daudé (3): tests/qtest/fuzz-test: Quit test_lp1878642 once done hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi() hw/scsi/megasas: Have incorrect cdb return MFI_STAT_ABORT_NOT_POSSIBLE hw/scsi/megasas.c | 6 ++ tests/qtest/fuzz-test.c | 197 2 files changed, 203 insertions(+) -- 2.26.2
qemu 6.0 rbd driver rewrite
Hi, i would like to submit a series for 6.0 which will convert the aio hooks to native coroutine hooks and add write zeroes support. The aio routines are nowadays just an emulation on top of coroutines which add additional overhead. For this I would like to lift the minimum librbd requirement to luminous release to get rid of the ifdef'ry in the code. Any objections? Best, Peter
Re: [PATCH v2] hw/nvme: Move NVMe emulation out of hw/block/ directory
On 11/30/20 9:52 PM, Klaus Jensen wrote:
> On Nov 30 15:52, Philippe Mathieu-Daudé wrote:
>> As IDE used to be, NVMe emulation is becoming an active
>> subsystem. Move it into its own namespace.
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> v2: Rebased after nvme-ns got merged in commit 8680d6e3646
>> ---
>> meson.build | 1 +
>> hw/{block/nvme.h => nvme/nvme-internal.h} | 4 +-
>> hw/{block => nvme}/nvme-ns.h | 0
>> hw/{block/nvme.c => nvme/core.c} | 2 +-
>> hw/{block => nvme}/nvme-ns.c | 0
>> MAINTAINERS | 2 +-
>> hw/Kconfig| 1 +
>> hw/block/Kconfig | 5 -
>> hw/block/meson.build | 1 -
>> hw/block/trace-events | 132 -
>> hw/meson.build| 1 +
>> hw/nvme/Kconfig | 4 +
>> hw/nvme/meson.build | 1 +
>> hw/nvme/trace-events | 133 ++
>> 14 files changed, 145 insertions(+), 142 deletions(-)
>> rename hw/{block/nvme.h => nvme/nvme-internal.h} (98%)
>> rename hw/{block => nvme}/nvme-ns.h (100%)
>> rename hw/{block/nvme.c => nvme/core.c} (99%)
>> rename hw/{block => nvme}/nvme-ns.c (100%)
>
> Would we want to consider renaming nvme-ns.c to namespace.c? And maybe
> also follow up with consolidating nvme-ns.h into nvme-internal.h?
Yes, good idea!
I'll respin.
Thanks,
Phil.
Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Paolo Bonzini writes:
> On 30/11/20 16:30, Daniel P. Berrangé wrote:
>> { 'struct': 'QCryptoSecretCommon',
>>'base': 'Object',
>>'state': { 'rawdata': '*uint8_t',
>> 'rawlen': 'size_t' },
>>'data': { '*format': 'QCryptoSecretFormat',
>> '*keyid': 'str',
>> '*iv': 'str' } }
>> { 'struct': 'QCryptoSecret',
>>'base': 'QCryptoSecretCommon',
>>'data': { '*data': 'str',
>> '*file': 'str' } }
>
> No, please don't do this. I want to keep writing C code, not a weird
> JSON syntax for C.
>
> I'd much rather have a FooOptions field in my struct so that I can
> just do visit_FooOptions in the UserCreatable.complete() method,
> that's feasible.
It should be, because it's what we've done elsewhere, isn't it?
Yes, we can extend QAPI to let us embed opaques outside the QAPI
schema's scope ("state"), along with means to create, destroy, and
clone. This is new.
But we can also invert: put the QAPI-generated C type ("config") in a
handwritten C type that marries it to "state".
Code to create and destroy is handwritten, and uses QAPI-generated code
for the "config" part.
A generic interface to handwritten creation is possible: Take the
QAPI-generated "config" type as argument, extract enough information to
call the appropriate constructor, return its value.
Generic destruction is also possible: all it needs is a map from
instance to destructor function.
None of this is new in QEMU.
