Re: [Qemu-devel] [PATCH V7 2/2] Add a new qmp command to do checkpoint, query xen replication status

[Zhang Chen]

On 02/22/2017 12:20 AM, Eric Blake wrote:
> On 02/21/2017 08:07 AM, Markus Armbruster wrote:
> > Zhang Chen <address@hidden> writes:
> >
> > > On 02/21/2017 07:15 PM, Markus Armbruster wrote:
> > > > Zhang Chen <address@hidden> writes:
> > > >
> > > > > We can call this qmp command to do checkpoint outside of qemu.
> > > > > Xen colo will need this function.
> > > > I know nothing about "Xen colo", but I'll try anyway.
> > > >
> > > > I *guess* "Xen colo" is a long-running activity, and the new commands
> > > > interact with it.  Correct?
> > > Yes, you are right.
> > We need to build a generic framework for interacting with long-running
> > activities.  But asking you to wait for it wouldn't be fair.
> >
> > > By the way this patch has been reviewed by Eric black.
> > >
> > > https://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03435.html
> Blake, but you're not the first to mis-type it.

I'm very sorry about this typo. =_=!.

> > > > > +{
> > > > > +    Error *err = NULL;
> > > > > +    ReplicationResult *result = g_new0(ReplicationResult, 1);
> > > > > +    replication_get_error_all(&err);
> > > > > +    result->status = err ?
> > > > > +                     REPLICATION_STATUS_ERROR :
> > > > > +                     REPLICATION_STATUS_NORMAL;
> > > > Reports only that there is an error, throws away the actual error
> > > > message.  Naive question: could the error message be good to know for
> > > > the QMP user?
> > > Yes, Xen colo will handle it.
> > Is that "yes, the QMP user could use the error message, but we're not
> > giving it to him regardless", or "no, the QMP user does not need to
> > know, and that's because we don't give it to him"?
> >
> > Even if we want QMP clients to treat all errors the same, transmitting
> > the error message can still be useful for troubleshooting.
> Ah, as in:
>
> if (err) {
>    result->status = REPLICATION_STATUS_ERROR;
>    result->has_desc = true;
>    result->desc = ...extract string from err
> } else {
>    result->status = REPLICATION_STATUS_NORMAL;
> }
>
> by modifying the JSON [1]

I got your point, but I don't understand this:

result->desc = ...extract string from err

Where we can get the string from err?
Can you give me a example?



> > > > > +##
> > > > > +{ 'enum': 'ReplicationStatus',
> > > > > +  'data': [ 'error', 'normal' ] }
> > > > Do you expect more status values in the future?
> > > >
> > > > If yes, what should clients do when they see a status value they don't
> > > > know?
> > > We will add other status for it, now, that's enough.
> > What should a QMP client do when it sees a ReplicationStatus value other
> > than 'error' and 'normal'?
> >
> > You need to provide some guidance, or else you won't be able to add
> > status values without breaking clients!
> >
> > > > If no, why not simply use bool?
> Off-hand, what other states do you envision adding?

OK, I will change it to bool in next version.

> > > > > +
> > > > > +##
> > > > > +# @ReplicationResult:
> > > > > +#
> > > > > +# The result format for 'query-xen-replication-status'.
> > > > > +#
> > > > > +# @status: enum of @ReplicationStatus, which shows current
> > > > > +#          replication error status
> > > > > +#
> > > > > +# Since: 2.9
> > > > > +##
> > > > > +{ 'struct': 'ReplicationResult',
> > > > > +  'data': { 'status': 'ReplicationStatus'} }
> [1] the modification would be:
>
> 'data': { 'status': 'ReplicationStatus', '*desc': 'str' }
>
> with documentation that @desc is #optional, is only for human
> consumption, and is only present when @status indicates an error.

I don't know how to get the 'desc'.
Please tell me.

Thanks
Zhang Chen

>






--
Thanks
Zhang Chen