[sheepdog] [PATCH] sheep: no need to set SD_FLAG_CMD_WRITE for local request

[MORITA Kazutaka]

At Tue, 28 Aug 2012 20:53:59 +0800,
Liu Yuan wrote:
> 
> On 08/28/2012 07:56 PM, MORITA Kazutaka wrote:
> > When sheep receives a requests without SD_FLAG_CMD_WRITE, begin_rx()
> > doesn't set req->data, so in such case prepare_cluster_msg() shouldn't
> > set the invalid data to msg->data, no?  I believe multicasting invalid
> > data makes it difficult for us to debug the code when we forget to add
> > SD_FLAG_CMD_WRITE.  If it is true, we need to add SD_FLAG_CMD_WRITE
> > even when the request is a local one because prepare_cluster_msg()
> > cannot know whether the request is from local or not.
> > 
> 
> It is internal function, so I don't think we need so strict checks. It seems that
> for now we always send data for cluster requests. So this check just slow down

The cost of the SD_FLAG_CMD_WRITE check is much cheaper than the one
of multicasting.

> things. Even if it send the invalid data by accident, these data will be ignored
> because the programmer write the request handler.

Note that all cluster operations are not internal ones.  For example,
it would help developers to return SD_RES_INALID_PARMS when the
cluster request doesn't have SD_FLAG_CMD_WRITE but its data_length is
not zero.

> 
> What cause most confusion to me earlier is that, why need to set CMD_WRITE flag when
> I already set opcode as WRITE.
> 
> > I think it is worth considering to remove SD_FLAG_CMD_WRITE
> > completely, but it is another issue to be discussed.  With the current
> > design, we should add the flag when sending data strictly, at least
> > for now.  With this patch, notify_recovery_completion_work() looks
> > like trying to receive some data with SD_OP_COMPLETE_RECOVERY to me...
> 
> If we want to use SD_FLAG_CMD_WRITE to indicate data direction, we'd better use another
> name such as SD_FLAG_DATA_IN/OUT. I guess I am not the only to ask why when setting something
> as WRITE for twice in a request.

Using SD_FLAG_DATA_IN/OUT is also okay to me, but it's also different
issue from this patch.  The problem of this patch is using a different
rule against local requests, and it is confusing.  At least with the
current design, we should add SD_FLAG_CMD_WRITE when sending data.

Thanks,

Kazutaka