| Message ID | 20130724141453.GG23378@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
T24gV2VkLCAyMDEzLTA3LTI0IGF0IDEwOjE0IC0wNDAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6 DQo+IEZyb206ICJKLiBCcnVjZSBGaWVsZHMiIDxiZmllbGRzQHJlZGhhdC5jb20+DQo+IA0KPiBU aGUgY2FsY3VsYXRpb24gb2YgYXR0cmlidXRlIGxlbmd0aCBmaWVsZHMgaXMgdG9vIGhpZ2ggYnkg Zm91ciBiZWNhdXNlDQo+IGl0IGluY29ycmVjdGx5IGluY2x1ZGVzIHRoZSBsZW5ndGggZmllbGQg aXRzZWxmLg0KPiANCj4gVGhpcyByZWdyZXNzaW9uIHdhcyBpbnRyb2R1Y2VkIGJ5DQo+IGI0YTJj Zjc2YWI3YzA4NjI4YzYyYjIwNjJkYWNlZmE0OTZiNTlkZmQgIk5GU3Y0OiBGaXggYSByZWdyZXNz aW9uDQo+IGFnYWluc3QgdGhlIEZyZWVCU0Qgc2VydmVyIiBhbmQgY2F1c2VzIE9QRU5zIHRvIHRo ZSBMaW51eCBORlMgc2VydmVyIHRvDQo+IGZhaWwgd2l0aCBCQURYRFIgZXJyb3JzICh0cmFuc2xh dGVkIGJ5IHRoZSBjbGllbnQgaW50byBFSU8pLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogSi4gQnJ1 Y2UgRmllbGRzIDxiZmllbGRzQHJlZGhhdC5jb20+DQo+IC0tLQ0KPiAgZnMvbmZzL25mczR4ZHIu YyB8ICAgIDIgKy0NCj4gIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKSwgMSBkZWxldGlv bigtKQ0KPiANCj4gSSB0aG91Z2h0IHlvdSBndXlzIGhhZCBhdXRvbWF0ZWQgdGVzdGluZyBhZ2Fp bnN0IHRoZSBMaW51eCBzZXJ2ZXI/ICBIb3cNCj4gZGlkIHRoaXMgc2xpcCB0aHJvdWdoIGludG8g dXBzdHJlYW0/DQo+IA0KPiAtLWIuDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczR4ZHIu YyBiL2ZzL25mcy9uZnM0eGRyLmMNCj4gaW5kZXggYzc0ZDYxNi4uZDZkNjc1NCAxMDA2NDQNCj4g LS0tIGEvZnMvbmZzL25mczR4ZHIuYw0KPiArKysgYi9mcy9uZnMvbmZzNHhkci5jDQo+IEBAIC0x MTE4LDcgKzExMTgsNyBAQCBzdGF0aWMgdm9pZCBlbmNvZGVfYXR0cnMoc3RydWN0IHhkcl9zdHJl YW0gKnhkciwgY29uc3Qgc3RydWN0IGlhdHRyICppYXAsDQo+ICAJCQkJbGVuLCAoKGNoYXIgKilw IC0gKGNoYXIgKilxKSArIDQpOw0KPiAgCQlCVUcoKTsNCj4gIAl9DQo+IC0JbGVuID0gKGNoYXIg KilwIC0gKGNoYXIgKilxIC0gKGJtdmFsX2xlbiA8PCAyKTsNCj4gKwlsZW4gPSAoY2hhciAqKXAg LSAoY2hhciAqKXEgLSAoYm12YWxfbGVuICsgMSA8PCAyKTsNCj4gIAkqcSsrID0gaHRvbmwoYm12 YWwwKTsNCj4gIAkqcSsrID0gaHRvbmwoYm12YWwxKTsNCj4gIAlpZiAoYm12YWxfbGVuID09IDMp DQoNClBsZWFzZSBzZWUgY29tbWl0IDRmM2NjNDgwOWE5OGExNjVhOTcwOGI3MmI0N2RlNzE2NDM3 OTdiYmQgKE5GU3Y0OiBGaXgNCmJyYWluZmFydCBpbiBhdHRyaWJ1dGUgbGVuZ3RoIGNhbGN1bGF0 aW9uKSB1cHN0cmVhbS4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQg bWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0 YXBwLmNvbQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 24, 2013 at 02:30:58PM +0000, Myklebust, Trond wrote: > On Wed, 2013-07-24 at 10:14 -0400, J. Bruce Fields wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > The calculation of attribute length fields is too high by four because > > it incorrectly includes the length field itself. > > > > This regression was introduced by > > b4a2cf76ab7c08628c62b2062dacefa496b59dfd "NFSv4: Fix a regression > > against the FreeBSD server" and causes OPENs to the Linux NFS server to > > fail with BADXDR errors (translated by the client into EIO). > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > fs/nfs/nfs4xdr.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > I thought you guys had automated testing against the Linux server? How > > did this slip through into upstream? > > > > --b. > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > index c74d616..d6d6754 100644 > > --- a/fs/nfs/nfs4xdr.c > > +++ b/fs/nfs/nfs4xdr.c > > @@ -1118,7 +1118,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, > > len, ((char *)p - (char *)q) + 4); > > BUG(); > > } > > - len = (char *)p - (char *)q - (bmval_len << 2); > > + len = (char *)p - (char *)q - (bmval_len + 1 << 2); > > *q++ = htonl(bmval0); > > *q++ = htonl(bmval1); > > if (bmval_len == 3) > > Please see commit 4f3cc4809a98a165a9708b72b47de71643797bbd (NFSv4: Fix > brainfart in attribute length calculation) upstream. Oh, good, thanks! But I still don't understand how this made it into upstream in the first place. Any NFSv4 testing at all against the Linux server would catch this, and I thought you had automated testing set up that you could run before submission. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-07-24 at 10:38 -0400, J. Bruce Fields wrote: > On Wed, Jul 24, 2013 at 02:30:58PM +0000, Myklebust, Trond wrote: > > On Wed, 2013-07-24 at 10:14 -0400, J. Bruce Fields wrote: > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > The calculation of attribute length fields is too high by four because > > > it incorrectly includes the length field itself. > > > > > > This regression was introduced by > > > b4a2cf76ab7c08628c62b2062dacefa496b59dfd "NFSv4: Fix a regression > > > against the FreeBSD server" and causes OPENs to the Linux NFS server to > > > fail with BADXDR errors (translated by the client into EIO). > > > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > --- > > > fs/nfs/nfs4xdr.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > I thought you guys had automated testing against the Linux server? How > > > did this slip through into upstream? > > > > > > --b. > > > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > > index c74d616..d6d6754 100644 > > > --- a/fs/nfs/nfs4xdr.c > > > +++ b/fs/nfs/nfs4xdr.c > > > @@ -1118,7 +1118,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, > > > len, ((char *)p - (char *)q) + 4); > > > BUG(); > > > } > > > - len = (char *)p - (char *)q - (bmval_len << 2); > > > + len = (char *)p - (char *)q - (bmval_len + 1 << 2); > > > *q++ = htonl(bmval0); > > > *q++ = htonl(bmval1); > > > if (bmval_len == 3) > > > > Please see commit 4f3cc4809a98a165a9708b72b47de71643797bbd (NFSv4: Fix > > brainfart in attribute length calculation) upstream. > > Oh, good, thanks! > > But I still don't understand how this made it into upstream in the first > place. > > Any NFSv4 testing at all against the Linux server would catch this, and > I thought you had automated testing set up that you could run before > submission. Yes, however it was truly a brainfart: the patch was never tested independently of the cleanup which removes the backfilling altogether. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On Wed, Jul 24, 2013 at 02:41:54PM +0000, Myklebust, Trond wrote: > On Wed, 2013-07-24 at 10:38 -0400, J. Bruce Fields wrote: > > On Wed, Jul 24, 2013 at 02:30:58PM +0000, Myklebust, Trond wrote: > > > On Wed, 2013-07-24 at 10:14 -0400, J. Bruce Fields wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > > > The calculation of attribute length fields is too high by four because > > > > it incorrectly includes the length field itself. > > > > > > > > This regression was introduced by > > > > b4a2cf76ab7c08628c62b2062dacefa496b59dfd "NFSv4: Fix a regression > > > > against the FreeBSD server" and causes OPENs to the Linux NFS server to > > > > fail with BADXDR errors (translated by the client into EIO). > > > > > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > > --- > > > > fs/nfs/nfs4xdr.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > I thought you guys had automated testing against the Linux server? How > > > > did this slip through into upstream? > > > > > > > > --b. > > > > > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > > > index c74d616..d6d6754 100644 > > > > --- a/fs/nfs/nfs4xdr.c > > > > +++ b/fs/nfs/nfs4xdr.c > > > > @@ -1118,7 +1118,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, > > > > len, ((char *)p - (char *)q) + 4); > > > > BUG(); > > > > } > > > > - len = (char *)p - (char *)q - (bmval_len << 2); > > > > + len = (char *)p - (char *)q - (bmval_len + 1 << 2); > > > > *q++ = htonl(bmval0); > > > > *q++ = htonl(bmval1); > > > > if (bmval_len == 3) > > > > > > Please see commit 4f3cc4809a98a165a9708b72b47de71643797bbd (NFSv4: Fix > > > brainfart in attribute length calculation) upstream. > > > > Oh, good, thanks! > > > > But I still don't understand how this made it into upstream in the first > > place. > > > > Any NFSv4 testing at all against the Linux server would catch this, and > > I thought you had automated testing set up that you could run before > > submission. > > Yes, however it was truly a brainfart: the patch was never tested > independently of the cleanup which removes the backfilling altogether. Would it be possible to fix the testing process so that it takes exactly the commit that you're sending in the pull request? --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index c74d616..d6d6754 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -1118,7 +1118,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, len, ((char *)p - (char *)q) + 4); BUG(); } - len = (char *)p - (char *)q - (bmval_len << 2); + len = (char *)p - (char *)q - (bmval_len + 1 << 2); *q++ = htonl(bmval0); *q++ = htonl(bmval1); if (bmval_len == 3)