[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v2 05/11] blockjobs: split interface into public

From: John Snow
Subject: Re: [Qemu-block] [PATCH v2 05/11] blockjobs: split interface into public/private
Date: Wed, 5 Oct 2016 12:20:50 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

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 <address@hidden>

--- 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.


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]