From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1browG-0007yg-2g for qemu-devel@nongnu.org; Wed, 05 Oct 2016 12:21:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1browD-0002k7-M1 for qemu-devel@nongnu.org; Wed, 05 Oct 2016 12:20:58 -0400 References: <1475272849-19990-1-git-send-email-jsnow@redhat.com> <1475272849-19990-6-git-send-email-jsnow@redhat.com> <20161005141708.GD1657@noname.str.redhat.com> From: John Snow Message-ID: Date: Wed, 5 Oct 2016 12:20:50 -0400 MIME-Version: 1.0 In-Reply-To: <20161005141708.GD1657@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 05/11] blockjobs: split interface into public/private List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-block@nongnu.org, vsementsov@virtuozzo.com, famz@redhat.com, stefanha@redhat.com, jcody@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org On 10/05/2016 10:17 AM, Kevin Wolf wrote: > Am 01.10.2016 um 00:00 hat John Snow geschrieben: >> To make it a little more obvious which functions are intended to be >> public interface and which are intended to be for use only by jobs >> themselves, split the interface into "public" and "private" files. >> >> Convert blockjobs (e.g. block/backup) to using the private interface. >> Leave blockdev and others on the public interface. >> >> Give up and let qemu-img use the internal interface, though it doesn't >> strictly need to be using it. >> >> As a side-effect, hide the BlockJob and BlockJobDriver implementation >> from most of the QEMU codebase. >> >> Signed-off-by: John Snow > >> --- a/block/replication.c >> +++ b/block/replication.c >> @@ -15,7 +15,7 @@ >> #include "qemu/osdep.h" >> #include "qemu-common.h" >> #include "block/nbd.h" >> -#include "block/blockjob.h" >> +#include "block/blockjob_int.h" >> #include "block/block_int.h" >> #include "block/block_backup.h" >> #include "sysemu/block-backend.h" > > This one is wrong. replication.c is a block job user, not part of the > implementation. After removing this hunk, the result still compiles. > Sorry, this is a mistake from an intermediate form of the patch. >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -7,16 +7,15 @@ >> #include "qemu/coroutine.h" >> #include "block/accounting.h" >> #include "block/dirty-bitmap.h" >> +#include "block/blockjob.h" >> #include "qapi/qmp/qobject.h" >> #include "qapi-types.h" >> #include "qemu/hbitmap.h" > > Hm... This header file doesn't need any of the definitions from > blockjob.h, so I guess .c files should include it explicitly rather than > getting it from here. That would be a separate patch, though. > OK. The reason this happened was because we used to have the struct definitions in block_int.h; and by moving those to a Blockjob-specific header, I needed to reintroduce those definitions somehow. I reasoned that BlockJobs were part of the public interface for the Block layer, so I added it here. I can remove it and include the public blockjob interface only where specifically required if desired, though. >> /* block.c */ >> typedef struct BlockDriver BlockDriver; >> -typedef struct BlockJob BlockJob; >> typedef struct BdrvChild BdrvChild; >> typedef struct BdrvChildRole BdrvChildRole; >> -typedef struct BlockJobTxn BlockJobTxn; >> >> typedef struct BlockDriverInfo { >> /* in bytes, 0 if irrelevant */ > >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -37,7 +37,7 @@ >> #include "sysemu/sysemu.h" >> #include "sysemu/block-backend.h" >> #include "block/block_int.h" >> -#include "block/blockjob.h" >> +#include "block/blockjob_int.h" >> #include "block/qapi.h" >> #include "crypto/init.h" >> #include "trace/control.h" > > qemu-img doesn't compile without including blockjob_int.h, but that's > just a sign that the split isn't completely right yet. Maybe it needs to > use the pulic function block_job_query() to get the information it takes > directly from the BlockJob struct. > > Kevin > Yes, you caught me. I'd need to spend a little more time shining up qemu-img, which just takes time away from that FDC rev-- I'm kidding. I'll fix this up at your request. --js