| Message ID | 1427276174-9130-7-git-send-email-wency@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 25/03/2015 10:36, Wen Congyang wrote: > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > block/nbd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/block/nbd.c b/block/nbd.c > index 3faf865..753b322 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState *bs) > bs->full_open_options = opts; > } > > +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode, > + Error **errp) > +{ > + BDRVNBDState *s = bs->opaque; > + > + /* > + * TODO: support COLO_SECONDARY_MODE if we allow secondary > + * QEMU becoming primary QEMU. > + */ > + if (mode != COLO_MODE_PRIMARY) { > + error_set(errp, QERR_INVALID_PARAMETER, "mode"); > + return; > + } > + > + if (s->connected) { > + error_setg(errp, "The connection is established"); > + return; > + } > + > + /* TODO: NBD client should be one child of quorum, how to verify it? */ > + nbd_connect_server(bs, errp); > +} > + > +static void nbd_do_checkpoint(BlockDriverState *bs, Error **errp) > +{ > + BDRVNBDState *s = bs->opaque; > + > + if (!s->connected) { > + error_setg(errp, "The connection is not established"); > + return; > + } > +} > + > +static void nbd_stop_replication(BlockDriverState *bs, Error **errp) > +{ > + BDRVNBDState *s = bs->opaque; > + > + if (!s->connected) { > + error_setg(errp, "The connection is not established"); > + return; > + } > + > + nbd_client_close(bs); > + s->connected = false; > +} > + > static BlockDriver bdrv_nbd = { > .format_name = "nbd", > .protocol_name = "nbd", > @@ -527,6 +573,9 @@ static BlockDriver bdrv_nbd_colo = { > .bdrv_detach_aio_context = nbd_detach_aio_context, > .bdrv_attach_aio_context = nbd_attach_aio_context, > .bdrv_refresh_filename = nbd_refresh_filename, > + .bdrv_start_replication = nbd_start_replication, > + .bdrv_do_checkpoint = nbd_do_checkpoint, > + .bdrv_stop_replication = nbd_stop_replication, > > .has_variable_length = true, > }; > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On Wed, 03/25 17:36, Wen Congyang wrote: > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > block/nbd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/block/nbd.c b/block/nbd.c > index 3faf865..753b322 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState *bs) > bs->full_open_options = opts; > } > > +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode, > + Error **errp) > +{ > + BDRVNBDState *s = bs->opaque; > + > + /* > + * TODO: support COLO_SECONDARY_MODE if we allow secondary > + * QEMU becoming primary QEMU. > + */ > + if (mode != COLO_MODE_PRIMARY) { > + error_set(errp, QERR_INVALID_PARAMETER, "mode"); Please use error_setg. (Please grep the whole series :) Fam
On 03/26/2015 03:21 PM, Fam Zheng wrote: > On Wed, 03/25 17:36, Wen Congyang wrote: >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> --- >> block/nbd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/block/nbd.c b/block/nbd.c >> index 3faf865..753b322 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState *bs) >> bs->full_open_options = opts; >> } >> >> +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode, >> + Error **errp) >> +{ >> + BDRVNBDState *s = bs->opaque; >> + >> + /* >> + * TODO: support COLO_SECONDARY_MODE if we allow secondary >> + * QEMU becoming primary QEMU. >> + */ >> + if (mode != COLO_MODE_PRIMARY) { >> + error_set(errp, QERR_INVALID_PARAMETER, "mode"); > > Please use error_setg. (Please grep the whole series :) Why? QERR_INVALID_PARAMETER includes ERROR_CLASS_GENERIC_ERROR. Thanks Wen Congyang > > Fam > . >
On Thu, 03/26 15:32, Wen Congyang wrote: > On 03/26/2015 03:21 PM, Fam Zheng wrote: > > On Wed, 03/25 17:36, Wen Congyang wrote: > >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> > >> --- > >> block/nbd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 49 insertions(+) > >> > >> diff --git a/block/nbd.c b/block/nbd.c > >> index 3faf865..753b322 100644 > >> --- a/block/nbd.c > >> +++ b/block/nbd.c > >> @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState *bs) > >> bs->full_open_options = opts; > >> } > >> > >> +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode, > >> + Error **errp) > >> +{ > >> + BDRVNBDState *s = bs->opaque; > >> + > >> + /* > >> + * TODO: support COLO_SECONDARY_MODE if we allow secondary > >> + * QEMU becoming primary QEMU. > >> + */ > >> + if (mode != COLO_MODE_PRIMARY) { > >> + error_set(errp, QERR_INVALID_PARAMETER, "mode"); > > > > Please use error_setg. (Please grep the whole series :) > > Why? QERR_INVALID_PARAMETER includes ERROR_CLASS_GENERIC_ERROR. Because error classes are deprecated. See also commit 5b347c5410. Fam
On 03/27/2015 09:06 AM, Fam Zheng wrote: > On Thu, 03/26 15:32, Wen Congyang wrote: >> On 03/26/2015 03:21 PM, Fam Zheng wrote: >>> On Wed, 03/25 17:36, Wen Congyang wrote: >>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>> --- >>>> block/nbd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 49 insertions(+) >>>> >>>> diff --git a/block/nbd.c b/block/nbd.c >>>> index 3faf865..753b322 100644 >>>> --- a/block/nbd.c >>>> +++ b/block/nbd.c >>>> @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState *bs) >>>> bs->full_open_options = opts; >>>> } >>>> >>>> +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode, >>>> + Error **errp) >>>> +{ >>>> + BDRVNBDState *s = bs->opaque; >>>> + >>>> + /* >>>> + * TODO: support COLO_SECONDARY_MODE if we allow secondary >>>> + * QEMU becoming primary QEMU. >>>> + */ >>>> + if (mode != COLO_MODE_PRIMARY) { >>>> + error_set(errp, QERR_INVALID_PARAMETER, "mode"); >>> >>> Please use error_setg. (Please grep the whole series :) >> >> Why? QERR_INVALID_PARAMETER includes ERROR_CLASS_GENERIC_ERROR. > > Because error classes are deprecated. See also commit 5b347c5410. I see. Will fix it in the next version. Thanks Wen Congyang > > Fam > . >
Fam Zheng <famz@redhat.com> writes: > On Thu, 03/26 15:32, Wen Congyang wrote: >> On 03/26/2015 03:21 PM, Fam Zheng wrote: >> > On Wed, 03/25 17:36, Wen Congyang wrote: >> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> >> --- >> >> block/nbd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 49 insertions(+) >> >> >> >> diff --git a/block/nbd.c b/block/nbd.c >> >> index 3faf865..753b322 100644 >> >> --- a/block/nbd.c >> >> +++ b/block/nbd.c >> >> @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState *bs) >> >> bs->full_open_options = opts; >> >> } >> >> >> >> +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode, >> >> + Error **errp) >> >> +{ >> >> + BDRVNBDState *s = bs->opaque; >> >> + >> >> + /* >> >> + * TODO: support COLO_SECONDARY_MODE if we allow secondary >> >> + * QEMU becoming primary QEMU. >> >> + */ >> >> + if (mode != COLO_MODE_PRIMARY) { >> >> + error_set(errp, QERR_INVALID_PARAMETER, "mode"); >> > >> > Please use error_setg. (Please grep the whole series :) >> >> Why? QERR_INVALID_PARAMETER includes ERROR_CLASS_GENERIC_ERROR. > > Because error classes are deprecated. See also commit 5b347c5410. Yes, new code should use ERROR_CLASS_GENERIC_ERROR, preferably through error_setg() or similar. The QERR_ macros in qerror.h expand to *two* expressions CLS, FMT, for use as function arguments. Dirty. Was done that way when error classes were introduced to reduce churn (commit 8546505..0f32cf6). Besides being dirty, they hide the error class. Bad, because we really want unusual things like a funky error class stand out right where the error is created. If they don't, we're prone to accidental uses of funky error classes creeping in. So far, we've eliminated all QERR_ macros using a funky error class but one: QERR_DEVICE_NOT_FOUND. That one needs to go, too. I've got patches in my queue for 2.4. In fact, all of qerror.h needs to go. I've got patches for 2.4 to clean it out except for a bunch of QERR_. Replacing these isn't hard, just prone to conflicts, so I'm saving it for a little later. For now, adding uses of QERR_ macros other than QERR_DEVICE_NOT_FOUND is still okay, it just creates a little extra work for me. But avoiding them makes their ultimate removal a bit easier, and is encouraged.
diff --git a/block/nbd.c b/block/nbd.c index 3faf865..753b322 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState *bs) bs->full_open_options = opts; } +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode, + Error **errp) +{ + BDRVNBDState *s = bs->opaque; + + /* + * TODO: support COLO_SECONDARY_MODE if we allow secondary + * QEMU becoming primary QEMU. + */ + if (mode != COLO_MODE_PRIMARY) { + error_set(errp, QERR_INVALID_PARAMETER, "mode"); + return; + } + + if (s->connected) { + error_setg(errp, "The connection is established"); + return; + } + + /* TODO: NBD client should be one child of quorum, how to verify it? */ + nbd_connect_server(bs, errp); +} + +static void nbd_do_checkpoint(BlockDriverState *bs, Error **errp) +{ + BDRVNBDState *s = bs->opaque; + + if (!s->connected) { + error_setg(errp, "The connection is not established"); + return; + } +} + +static void nbd_stop_replication(BlockDriverState *bs, Error **errp) +{ + BDRVNBDState *s = bs->opaque; + + if (!s->connected) { + error_setg(errp, "The connection is not established"); + return; + } + + nbd_client_close(bs); + s->connected = false; +} + static BlockDriver bdrv_nbd = { .format_name = "nbd", .protocol_name = "nbd", @@ -527,6 +573,9 @@ static BlockDriver bdrv_nbd_colo = { .bdrv_detach_aio_context = nbd_detach_aio_context, .bdrv_attach_aio_context = nbd_attach_aio_context, .bdrv_refresh_filename = nbd_refresh_filename, + .bdrv_start_replication = nbd_start_replication, + .bdrv_do_checkpoint = nbd_do_checkpoint, + .bdrv_stop_replication = nbd_stop_replication, .has_variable_length = true, };