Re: [Qemu-devel] [Qemu-block] [PATCH v3 2/6] replication: add shared-disk and shared-disk-id options

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

On Wed, Mar 01, 2017 at 11:53:14AM +0800, Hailiang Zhang wrote:
> On 2017/2/28 1:10, Stefan Hajnoczi wrote:
> > On Fri, Jan 20, 2017 at 11:47:56AM +0800, zhanghailiang wrote:
> > > @@ -119,12 +136,31 @@ static int replication_open(BlockDriverState *bs, QDict *options,
> > >                      "The option mode's value should be primary or secondary");
> > >           goto fail;
> > >       }
> > > +    s->is_shared_disk = qemu_opt_get_bool(opts, REPLICATION_SHARED_DISK,
> > > +                                          false);
> > > +    if (s->is_shared_disk && (s->mode == REPLICATION_MODE_PRIMARY)) {
> > > +        shared_disk_id = qemu_opt_get(opts, REPLICATION_SHARED_DISK_ID);
> > > +        if (!shared_disk_id) {
> > > +            error_setg(&local_err, "Missing shared disk blk option");
> > > +            goto fail;
> > > +        }
> > > +        s->shared_disk_id = g_strdup(shared_disk_id);
> > > +        blk = blk_by_name(s->shared_disk_id);
> > > +        if (!blk) {
> > > +            error_setg(&local_err, "There is no %s block", s->shared_disk_id);
> > > +            goto fail;
> > > +        }
> > > +        /* We can't access root member of BlockBackend directly */
> > > +        tmp_bs = blk_bs(blk);
> > > +        s->primary_disk = QLIST_FIRST(&tmp_bs->parents);
> > 
> > Why is this necessary?
> > 
> > We already have the BlockBackend for the primary disk.  I'm not sure why
> > the BdrvChild is needed.
> > 
> 
> Er, the helper backup_job_create() needs the BlockDriverState for the primary disk,

You can get the BDS any time using blk_bs().

> besides, we want to make it same with 'secondary_disk' in struct BDRVReplicationState.

Block drivers are allowed to access BDS->parents so it's not necessarily
a problem that the code does this.  But it's rare and usually means
there is a non-standard relationship between BDS nodes.

It wasn't obvious from the code why you are going through the trouble of
getting a BdrvChild instead of using the BlockBackend (the normal way of
accessing a drive).

Perhaps a comment would help:

/* We have a BlockBackend for the primary disk but use BdrvChild for
 * consistency - active_disk, secondary_disk, etc are also BdrvChild.
 */

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]