Kevin Wolf <kw...@redhat.com> writes: > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >> Laszlo Ersek <ler...@redhat.com> writes: >> > This one has got to be one of the longest bike-shedding sessions! :) >> > >> > I'm fine with this patch, but I could suggest two improvements. >> > >> > (1) When blk_getlength() fails, we could format the negative error code >> > returned by it into the error message. >> >> I can do that. > > By using error_setg_errno(), I assume. Not throwing away error details > is always good. > >> > (2) We could extract the common code to a new function in >> > "hw/block/block.c". (It says "Common code for block device models" on >> > the tin.) >> >> There's so much common code in these two files even before this patch... > > My understanding is that hw/block/block.c contains code that is > potentially useful to all kinds of block devices, not random code that > two specific similar devices happen to share. > > If we want to deduplicate some code in the flash devices, without any > expectation that other devices will use it at some point, I'd rather > create a new source file hw/block/pflash_common.c or something like > that.
Yes. The helper I came up with (appended) isn't really specific to flash devices. Would it be okay for hw/block/block.c even though only the two flash devices use it for now? bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size, Error **errp) { int64_t blk_len; int ret; blk_len = blk_getlength(blk); if (blk_len < 0) { error_setg_errno(errp, -blk_len, "can't get size of block backend '%s'", blk_name(blk)); return false; } if (blk_len != size) { error_setg(errp, "device requires %" PRIu64 " bytes, " "block backend '%s' provides %" PRIu64 " bytes", size, blk_name(blk), blk_len); return false; } /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */ assert(size <= BDRV_REQUEST_MAX_BYTES); ret = blk_pread(blk, 0, buf, size); if (ret < 0) { error_setg_errno(errp, -ret, "can't read block backend '%s'", blk_name(blk)); return false; } return true; }