[Qemu-devel] [PATCH] 9pfs: Stat response from server offset by 2 bytes

[Qemu-devel] [PATCH] 9pfs: Stat response from server offset by 2 bytes

[David Gibson]

From: Timothy Rule <trule@linux.vnet.ibm.com>

The 9P spec states that for the stat message the "stat[n]" structure shall be
encoded at offset 7 in the 9P message (see §13.9 message Rstat).

The existing code is encoding a 2 byte value (hard coded 0 value) at
offset 7 of the 9P message, and then follows with the "stat[n]" structure
at offset 9 of the 9P message.

This patch removes the encoding of the 2 byte value which has the effect
of moving the "stat[n]" structure from offset 9 to offset 7 in the 9P
message Rstat.

Signed-off-by: Timothy Rule <trule@linux.vnet.ibm.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/9pfs/virtio-9p.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 01cf337..35d8851 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -1279,7 +1279,7 @@ static void v9fs_stat(void *opaque)
     if (err < 0) {
         goto out;
     }
-    offset += pdu_marshal(pdu, offset, "wS", 0, &v9stat);
+    offset += pdu_marshal(pdu, offset, "S", &v9stat);
     err = offset;
     trace_v9fs_stat_return(pdu->tag, pdu->id, v9stat.mode,
                            v9stat.atime, v9stat.mtime, v9stat.length);
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH] 9pfs: Stat response from server offset by 2 bytes

[Aneesh Kumar K.V]

On Tue,  8 Nov 2011 16:55:17 +1100, David Gibson <david@gibson.dropbear.id.au> wrote:
> From: Timothy Rule <trule@linux.vnet.ibm.com>
> 
> The 9P spec states that for the stat message the "stat[n]" structure shall be
> encoded at offset 7 in the 9P message (see §13.9 message Rstat).
> 
> The existing code is encoding a 2 byte value (hard coded 0 value) at
> offset 7 of the 9P message, and then follows with the "stat[n]" structure
> at offset 9 of the 9P message.
> 
> This patch removes the encoding of the 2 byte value which has the effect
> of moving the "stat[n]" structure from offset 9 to offset 7 in the 9P
> message Rstat.
> 
> Signed-off-by: Timothy Rule <trule@linux.vnet.ibm.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/9pfs/virtio-9p.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 01cf337..35d8851 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1279,7 +1279,7 @@ static void v9fs_stat(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    offset += pdu_marshal(pdu, offset, "wS", 0, &v9stat);
> +    offset += pdu_marshal(pdu, offset, "S", &v9stat);
>      err = offset;
>      trace_v9fs_stat_return(pdu->tag, pdu->id, v9stat.mode,
>                             v9stat.atime, v9stat.mtime, v9stat.length);

The reason for that "w" is explained in the 9p2000 protocol RFC. Towards
the end of "wstat" message para we have

BUGS

    To make the contents of a directory, such as returned by read(5),
    easy to parse, each directory entry begins with a size field. For
    consistency, the entries in Twstat and Rstat messages also contain
    their size, which means the size appears twice. For example, the
    Rstat message is formatted as ``(4+1+2+2+n)[4] Rstat tag[2] n[2]
    (n-2)[2] type[2] dev[4]...,'' where n is the value returned by
    convD2M.

Also on the client side we do in p9_client_stat

	err = p9pdu_readf(req->rc, clnt->proto_version, "wS", &ignored, ret);
	if (err) {

So the above change will break existing client.

Any reason why you need to make the above change ?

-aneesh

Re: [Qemu-devel] [PATCH] 9pfs: Stat response from server offset by 2 bytes

[Tim Rule]

On 11/08/2011 02:39 PM, Aneesh Kumar K.V wrote:
> On Tue,  8 Nov 2011 16:55:17 +1100, David Gibson<david@gibson.dropbear.id.au>  wrote:
>> From: Timothy Rule<trule@linux.vnet.ibm.com>
>>
>> The 9P spec states that for the stat message the "stat[n]" structure shall be
>> encoded at offset 7 in the 9P message (see §13.9 message Rstat).
>>
>> The existing code is encoding a 2 byte value (hard coded 0 value) at
>> offset 7 of the 9P message, and then follows with the "stat[n]" structure
>> at offset 9 of the 9P message.
>>
>> This patch removes the encoding of the 2 byte value which has the effect
>> of moving the "stat[n]" structure from offset 9 to offset 7 in the 9P
>> message Rstat.
>>
>> Signed-off-by: Timothy Rule<trule@linux.vnet.ibm.com>
>> Signed-off-by: David Gibson<david@gibson.dropbear.id.au>
>> ---
>>   hw/9pfs/virtio-9p.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
>> index 01cf337..35d8851 100644
>> --- a/hw/9pfs/virtio-9p.c
>> +++ b/hw/9pfs/virtio-9p.c
>> @@ -1279,7 +1279,7 @@ static void v9fs_stat(void *opaque)
>>       if (err<  0) {
>>           goto out;
>>       }
>> -    offset += pdu_marshal(pdu, offset, "wS", 0,&v9stat);
>> +    offset += pdu_marshal(pdu, offset, "S",&v9stat);
>>       err = offset;
>>       trace_v9fs_stat_return(pdu->tag, pdu->id, v9stat.mode,
>>                              v9stat.atime, v9stat.mtime, v9stat.length);
> The reason for that "w" is explained in the 9p2000 protocol RFC. Towards
> the end of "wstat" message para we have
>
> BUGS
>
>      To make the contents of a directory, such as returned by read(5),
>      easy to parse, each directory entry begins with a size field. For
>      consistency, the entries in Twstat and Rstat messages also contain
>      their size, which means the size appears twice. For example, the
>      Rstat message is formatted as ``(4+1+2+2+n)[4] Rstat tag[2] n[2]
>      (n-2)[2] type[2] dev[4]...,'' where n is the value returned by
>      convD2M.
>
> Also on the client side we do in p9_client_stat
>
> 	err = p9pdu_readf(req->rc, clnt->proto_version, "wS",&ignored, ret);
> 	if (err) {
>
> So the above change will break existing client.
>
> Any reason why you need to make the above change ?
>
> -aneesh
It would be fine for us to implement our solution that way. I apologise 
for the inconvenience, I wrote this particular function against the 
9P2000.u specification which does not mention the Bug.

Tim.