Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

[Changlong Xie]

On 02/24/2016 08:35 PM, Alberto Garcia wrote:
> On Wed 24 Feb 2016 11:11:54 AM CET, Changlong Xie wrote:
> > -static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
> > +static void quorum_report_bad(QuorumOpType type, QuorumAIOCB *acb,
> > +                              char *node_name, int ret)
> >   {
> >       const char *msg = NULL;
> >       if (ret < 0) {
> >           msg = strerror(-ret);
> >       }
> > -    qapi_event_send_quorum_report_bad(!!msg, msg, node_name,
> > -                                      acb->sector_num, acb->nb_sectors, 
> > &error_abort);
> > +
> > +    switch (type) {
> > +    case QUORUM_OP_TYPE_READ:
> > +    case QUORUM_OP_TYPE_WRITE:
> > +        qapi_event_send_quorum_report_bad(false, 0, !!msg, msg, node_name,
> > +                                          acb->sector_num, acb->nb_sectors,
> > +                                          &error_abort);
> > +        break;
> > +    case QUORUM_OP_TYPE_FLUSH:
> > +        qapi_event_send_quorum_report_bad(true, type, !!msg, msg, node_name,
> > +                                          0, 0, &error_abort);
> > +        break;
>
> A few comments:
>
>    - Why don't you set the 'type' field in read and write operations? You
>      defined all three values but you are only using 'flush' here.
>
>    - For the case of flush you could set sectors-count to the total size
>      of the BlockDriverState as Eric suggested (bdrv_nb_sectors(bs)).
>      Setting both to 0 could confuse clients that are not ready to deal
>      with flush failures.
>
>    - Since the QuorumAIOCB parameter is not used in the flush case, you
>      could replace it from the function prototype and use sector_num and
>      nb_sectors instead. That way you can also omit the switch.

Surely, all your suggestions are helpful. Also i will add "Reviewed-by" 
in patch 1/3, 3/3 in next version.

Thanks
        -Xie

> Berto
>
>
> .