Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
Peter Maydell writes:
> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé wrote:
>>
>> As we have no interest in the underlying block geometry,
>> directly call blk_getlength(). We have to care about machines
>> creating SD card with not drive attached (probably incorrect
>> API use). Simply emit a warning when such Frankenstein cards
>> of zero size are reset.
>
> Which machines create SD cards without a backing block device?
>
> I have a feeling that also the monitor "change" and "eject"
> commands can remove the backing block device from the SD card
> object.
Correct:
static const BlockDevOps sd_block_ops = {
.change_media_cb = sd_cardchange,
};
This is TYPE_SD_CARD's ("sd-card"). What exactly does that device
model?
If it's the sd-card, then the modelling is odd. A physical SD card gets
plugged and unplugged as a whole. This model can't. Instead, you
change "media". Isn't the SD card the medium?
The other device models with removable media (IDE & SCSI CD drives,
floppy drives) model the receptacle for media. On media change, the
drive stays put, and only the medium changes, both in the physical world
and in our virtual world.
Our "sd-card" seems to be an "SD card drive". But then I wonder what
the thing at the other end of TYPE_SD_BUS ("sd-bus") actually models.
Any ideas?
Re: [PATCH v11 7/8] nbd: introduce ERRP_AUTO_PROPAGATE
Vladimir Sementsov-Ogievskiy writes:
> If we want to add some info to errp (by error_prepend() or
> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
> Otherwise, this info will not be added when errp == _fatal
> (the program will exit prior to the error_append_hint() or
> error_prepend() call). Fix such cases.
>
> If we want to check error after errp-function call, we need to
> introduce local_err and then propagate it to errp. Instead, use
> ERRP_AUTO_PROPAGATE macro, benefits are:
> 1. No need of explicit error_propagate call
> 2. No need of explicit local_err variable: use errp directly
> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
>_fatal, this means that we don't break error_abort
>(we'll abort on error_set, not on error_propagate)
>
> This commit is generated by command
>
> sed -n '/^Network Block Device (NBD)$/,/^$/{s/^F: //p}' \
> MAINTAINERS | \
> xargs git ls-files | grep '\.[hc]$' | \
> xargs spatch \
> --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> --macro-file scripts/cocci-macro-file.h \
> --in-place --no-show-diff --max-width 80
>
> Reported-by: Kevin Wolf
> Reported-by: Greg Kurz
> Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Markus Armbruster
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
On 7/5/20 12:26 AM, Philippe Mathieu-Daudé wrote:
> On 7/5/20 12:18 AM, Philippe Mathieu-Daudé wrote:
>> On 7/5/20 12:10 AM, Philippe Mathieu-Daudé wrote:
>>> On 7/4/20 1:42 AM, Philippe Mathieu-Daudé wrote:
On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
> On 7/3/20 3:23 PM, Peter Maydell wrote:
>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé
>> wrote:
>>>
>>> As we have no interest in the underlying block geometry,
>>> directly call blk_getlength(). We have to care about machines
>>> creating SD card with not drive attached (probably incorrect
>>> API use). Simply emit a warning when such Frankenstein cards
>>> of zero size are reset.
>>
>> Which machines create SD cards without a backing block device?
>
> The Aspeed machines:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html
>>>
>>> Also all boards using:
>>>
>>> hw/sd/milkymist-memcard.c:278:/* FIXME use a qdev drive property
>>> instead of drive_get_next() */
>>> hw/sd/pl181.c:506:/* FIXME use a qdev drive property instead of
>>> drive_get_next() */
>>> hw/sd/ssi-sd.c:253:/* FIXME use a qdev drive property instead of
>>> drive_get_next() */
>>>
>>> I.e.:
>>>
>>> static void pl181_realize(DeviceState *dev, Error **errp)
>>> {
>>> PL181State *s = PL181(dev);
>>> DriveInfo *dinfo;
>>>
>>> /* FIXME use a qdev drive property instead of drive_get_next() */
>>> dinfo = drive_get_next(IF_SD);
>>> s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
>>> if (s->card == NULL) {
>>> error_setg(errp, "sd_init failed");
>>> }
>>> }
>>
>> Doh I was pretty sure this series was merged:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg514645.html
>>
>> Time to respin I guess, addressing your comment...
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html
>
> Not straight forward at first glance, so probably too late for soft
> freeze.
I looked in backups of my previous computer and found the branch with
your comment addressed... Not sure why I never send the series, but
this explains why I was sure it was already fixed.
>
> Meanwhile patches 1-8 are reviewed and sufficient to fix
> CVE-2020-13253. The problematic patch is #9, your "Check address is
> in range" suggestion. Patches 11-14 are also reviewed and can go in.
>
> Peter, can you consider taking them via your ARM queue? If you prefer
> I can prepare a pull request.
>
> I'll keep working on this during the next dev window.
>
> Thanks,
>
> Phil.
>
>>
>>>
>
>> I have a feeling that also the monitor "change" and "eject"
>> commands can remove the backing block device from the SD card
>> object.
>
> This is what I wanted to talk about on IRC. This seems wrong to me,
> we should eject the card and destroy it, and recreate a new card
> when plugging in another backing block device.
>
> Keep the reparenting on the bus layer, not on the card.
I was wrong, the current code is correct:
void sdbus_reparent_card(SDBus *from, SDBus *to)
{
SDState *card = get_card(from);
SDCardClass *sc;
bool readonly;
/* We directly reparent the card object rather than implementing this
* as a hotpluggable connection because we don't want to expose SD
cards
* to users as being hotpluggable, and we can get away with it in this
* limited use case. This could perhaps be implemented more cleanly in
* future by adding support to the hotplug infrastructure for "device
* can be hotplugged only via code, not by user".
*/
if (!card) {
return;
}
sc = SD_CARD_GET_CLASS(card);
readonly = sc->get_readonly(card);
sdbus_set_inserted(from, false);
qdev_set_parent_bus(DEVICE(card), >qbus);
sdbus_set_inserted(to, true);
sdbus_set_readonly(to, readonly);
}
What I don't understand is why create a sdcard with no block backend.
Maybe this is old code before the null-co block backend existed? I
haven't checked the git history yet.
I'll try to restrict sdcard with only block backend and see if
something break (I doubt) at least it simplifies the code.
But I need to update the Aspeed machines first.
The problem when not using block backend, is the size is 0,
so the next patch abort in sd_reset() due to:
static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
{
assert(addr < sd->size);
>>>
>>>
>>
>
Re: [PATCH v5 0/7] Introduce 'yank' oob qmp command to recover from hanging qemu
On Wed, 24 Jun 2020 21:47:46 +0200 Lukas Straub wrote: > On Tue, 23 Jun 2020 16:42:30 +0200 > Lukas Straub wrote: > > > Hello Everyone, > > In many cases, if qemu has a network connection (qmp, migration, chardev, > > etc.) > > to some other server and that server dies or hangs, qemu hangs too. > > These patches introduce the new 'yank' out-of-band qmp command to recover > > from > > these kinds of hangs. The different subsystems register callbacks which get > > executed with the yank command. For example the callback can shutdown() a > > socket. This is intended for the colo use-case, but it can be used for other > > things too of course. > > > > Regards, > > Lukas Straub > > > > v5: > > -move yank.c to util/ > > -move yank.h to include/qemu/ > > -add license to yank.h > > -use const char* > > -nbd: use atomic_store_release and atomic_load_aqcuire > > -io-channel: ensure thread-safety and document it > > -add myself as maintainer for yank > > > > v4: > > -fix build errors... > > > > v3: > > -don't touch softmmu/vl.c, use __contructor__ attribute instead (Paolo > > Bonzini) > > -fix build errors > > -rewrite migration patch so it actually passes all tests > > > > v2: > > -don't touch io/ code anymore > > -always register yank functions > > -'yank' now takes a list of instances to yank > > -'query-yank' returns a list of yankable instances > > > > Lukas Straub (7): > > Introduce yank feature > > block/nbd.c: Add yank feature > > chardev/char-socket.c: Add yank feature > > migration: Add yank feature > > io/channel-tls.c: make qio_channel_tls_shutdown thread-safe > > io: Document thread-safety of qio_channel_shutdown > > MAINTAINERS: Add myself as maintainer for yank feature > > > > MAINTAINERS | 13 +++ > > block/nbd.c | 101 --- > > chardev/char-socket.c | 24 + > > include/io/channel.h | 2 + > > include/qemu/yank.h | 79 +++ > > io/channel-tls.c | 6 +- > > migration/channel.c | 12 +++ > > migration/migration.c | 18 +++- > > migration/multifd.c | 10 ++ > > migration/qemu-file-channel.c | 6 ++ > > migration/savevm.c| 2 + > > qapi/misc.json| 45 + > > tests/Makefile.include| 2 +- > > util/Makefile.objs| 1 + > > util/yank.c | 179 ++ > > 15 files changed, 459 insertions(+), 41 deletions(-) > > create mode 100644 include/qemu/yank.h > > create mode 100644 util/yank.c > > > > -- > > 2.20.1 > > Forgot to cc Stefan Hajnoczi... Ping... pgpJ1LBrDzu30.pgp Description: OpenPGP digital signature
