From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45245) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7ErR-00019h-1f for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:18:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7ErJ-0007aY-HL for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:17:52 -0400 Received: from mail6.webfaction.com ([74.55.86.74]:34028 helo=smtp.webfaction.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7ErJ-0007a2-AC for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:17:45 -0400 Message-ID: <5202F1B7.5040405@ctshepherd.com> Date: Thu, 08 Aug 2013 02:17:43 +0100 From: Charlie Shepherd MIME-Version: 1.0 References: <1375728247-1306-1-git-send-email-charlie@ctshepherd.com> <1375728247-1306-5-git-send-email-charlie@ctshepherd.com> <20130806093628.GG3117@dhcp-200-207.str.redhat.com> In-Reply-To: <20130806093628.GG3117@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] Convert block functions to coroutine versions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, gabriel@kerneis.info, qemu-devel@nongnu.org, stefanha@gmail.com On 06/08/2013 10:36, Kevin Wolf wrote: > Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben: >> This patch follows on from the previous one and converts some block layer functions to be >> explicitly annotated with coroutine_fn instead of yielding depending upon calling context. >> --- >> block.c | 235 ++++++++++++++++++++++++++------------------------ >> block/mirror.c | 4 +- >> include/block/block.h | 37 ++++---- >> 3 files changed, 148 insertions(+), 128 deletions(-) >> >> diff --git a/block.c b/block.c >> index aaa122c..e7011f9 100644 >> --- a/block.c >> +++ b/block.c >> @@ -164,7 +164,7 @@ bool bdrv_io_limits_enabled(BlockDriverState *bs) >> || io_limits->iops[BLOCK_IO_LIMIT_TOTAL]; >> } >> >> -static void bdrv_io_limits_intercept(BlockDriverState *bs, >> +static void coroutine_fn bdrv_io_limits_intercept(BlockDriverState *bs, >> bool is_write, int nb_sectors) >> { >> int64_t wait_time = -1; >> @@ -364,7 +364,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name, >> >> typedef struct CreateCo { >> BlockDriver *drv; >> - char *filename; >> + const char *filename; >> QEMUOptionParameter *options; >> int ret; >> } CreateCo; > Like Gabriel said, this should be a separate patch. Keeping it in this > series is probably easiest. > >> @@ -372,48 +372,48 @@ typedef struct CreateCo { >> static void coroutine_fn bdrv_create_co_entry(void *opaque) >> { >> CreateCo *cco = opaque; >> - assert(cco->drv); >> - >> - cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options); >> + cco->ret = bdrv_create(cco->drv, cco->filename, cco->options); >> } >> >> -int bdrv_create(BlockDriver *drv, const char* filename, >> +int coroutine_fn bdrv_create(BlockDriver *drv, const char* filename, >> QEMUOptionParameter *options) >> { >> int ret; >> + char *dup_fn; >> + >> + assert(drv); >> + if (!drv->bdrv_co_create) { >> + return -ENOTSUP; >> + } >> >> + dup_fn = g_strdup(filename); >> + ret = drv->bdrv_co_create(dup_fn, options); >> + g_free(dup_fn); >> + return ret; >> +} >> + >> + >> +int bdrv_create_sync(BlockDriver *drv, const char* filename, >> + QEMUOptionParameter *options) > bdrv_foo_sync is an unfortunate naming convention, because > bdrv_pwrite_sync already exists and means something totally different: > It's a write directly followed by a disk flush. Yes it's not a great naming convention. I could rename bdrv_pwrite_sync to something like bdrv_pwrite_and_sync? Or is that too horrible? An alternative would be bdrv_foo and bdrv_sync_foo? But bdrv_sync_pwrite and bdrv_pwrite_sync would be quite confusing in a hurry. >> diff --git a/include/block/block.h b/include/block/block.h >> index dd8eca1..57d8ba2 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -125,9 +125,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top); >> void bdrv_delete(BlockDriverState *bs); >> int bdrv_parse_cache_flags(const char *mode, int *flags); >> int bdrv_parse_discard_flags(const char *mode, int *flags); >> -int bdrv_file_open(BlockDriverState **pbs, const char *filename, >> +int coroutine_fn bdrv_file_open(BlockDriverState **pbs, const char *filename, >> QDict *options, int flags); >> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options); >> +int coroutine_fn bdrv_open_backing_file(BlockDriverState *bs, QDict *options); >> int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, >> int flags, BlockDriver *drv); >> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, >> @@ -150,18 +150,24 @@ void bdrv_dev_eject_request(BlockDriverState *bs, bool force); >> bool bdrv_dev_has_removable_media(BlockDriverState *bs); >> bool bdrv_dev_is_tray_open(BlockDriverState *bs); >> bool bdrv_dev_is_medium_locked(BlockDriverState *bs); >> -int bdrv_read(BlockDriverState *bs, int64_t sector_num, >> +int coroutine_fn bdrv_read(BlockDriverState *bs, int64_t sector_num, >> uint8_t *buf, int nb_sectors); >> -int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num, >> +int bdrv_read_sync(BlockDriverState *bs, int64_t sector_num, >> + uint8_t *buf, int nb_sectors); >> +int coroutine_fn bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num, >> uint8_t *buf, int nb_sectors); >> -int bdrv_write(BlockDriverState *bs, int64_t sector_num, >> +int coroutine_fn bdrv_write(BlockDriverState *bs, int64_t sector_num, >> + const uint8_t *buf, int nb_sectors); >> +int bdrv_write_sync(BlockDriverState *bs, int64_t sector_num, >> const uint8_t *buf, int nb_sectors); >> -int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov); >> -int bdrv_pread(BlockDriverState *bs, int64_t offset, >> +int coroutine_fn bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov); >> +int coroutine_fn bdrv_pread(BlockDriverState *bs, int64_t offset, >> + void *buf, int count); >> +int bdrv_pread_sync(BlockDriverState *bs, int64_t offset, >> void *buf, int count); > I haven't checked everything, but bdrv_pread_sync is an example of a > declaration without any user and without implementation. > > Proper patch splitting will make review of such things easier, so I'll > defer thorough review until then. Yes my bad, hopefully splitting them as per your other email will catch these issues.