Merge lp:~julian-edwards/gwacl/extra-putpage-checks into lp:gwacl

[Julian Edwards]

When dealing with page blobs, make sure any sizes and ranges are aligned to 512 byte boundaries.

Add more consistency checks around dealing with page blobs. We have to make sure any sizes and ranges are aligned to 512 byte boundaries.

[Gavin Panella (allenap)]

Looks good. Couple of points.

[1]

+            return fmt.Errorf("Size must be multiple of 512 bytes")

*a* multiple?

Here and in one other place.

[2]

+    validStart := math.Mod(float64(req.StartRange), 512) == 0

There is a % operator in Go:

    validStart := (req.StartRange % 512 == 0)

review: Approve

[Julian Edwards (julian-edwards)]

On 24/06/13 18:31, Gavin Panella wrote:
> Review: Approve
>
> Looks good. Couple of points.
>
>
> [1]
>
> + return fmt.Errorf("Size must be multiple of 512 bytes")
>
> *a* multiple?
>
> Here and in one other place.

Omitting prepositions is perfectly normal English, but I will assuage
your concerns.

>
>
> [2]
>
> + validStart := math.Mod(float64(req.StartRange), 512) == 0
>
> There is a % operator in Go:
>
> validStart := (req.StartRange % 512 == 0)
>

Feck. Sometimes you look past the bleedin' obvious when a language
teaches you that you can't rely on the obvious.

Thanks!

[lp:~julian-edwards/gwacl/extra-putpage-checks]

132. By Julian Edwards on 2013-06-25

gavin's review comments

=== modified file 'storage_base.go'
--- storage_base.go	2013-06-24 14:05:24 +0000
+++ storage_base.go	2013-06-25 00:24:24 +0000
@@ -439,6 +439,9 @@
         if req.Size == 0 {
             return fmt.Errorf("Must supply a size for a page blob")
         }
+        if req.Size % 512 != 0 {
+            return fmt.Errorf("Size must be a multiple of 512 bytes")
+        }
     case "block":
         blobType = "BlockBlob"
     default:
@@ -477,6 +480,12 @@
 // Send a request to add a range of data into a page blob.
 // See http://msdn.microsoft.com/en-us/library/windowsazure/ee691975.aspx
 func (context *StorageContext) PutPage(req *PutPageRequest) error {
+    validStart := (req.StartRange % 512) == 0
+    validEnd := (req.EndRange % 512) == 511
+    if !(validStart && validEnd) {
+        return fmt.Errorf(
+            "StartRange must be a multiple of 512, EndRange must be one less than a multiple of 512")
+    }
     uri := addURLQueryParams(
         context.GetFileURL(req.Container, req.Filename),
         "comp", "page")

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-06-24 11:25:25 +0000
+++ storage_base_test.go	2013-06-25 00:24:24 +0000
@@ -664,7 +664,7 @@
 
     err := context.PutPage(&PutPageRequest{
         Container: "container", Filename: "filename", StartRange: 0,
-        EndRange: 512, Data: dataReader})
+        EndRange: 511, Data: dataReader})
     c.Assert(err, IsNil)
 
     // Ensure that container was set right.
@@ -672,7 +672,7 @@
     // Ensure that the Authorization header is set.
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
     // Check the range is set.
-    c.Check(transport.Request.Header.Get("x-ms-range"), Equals, "bytes=0-512")
+    c.Check(transport.Request.Header.Get("x-ms-range"), Equals, "bytes=0-511")
     // Check special page write header.
     c.Check(transport.Request.Header.Get("x-ms-page-write"), Equals, "update")
     // "?comp=page" should be part of the URL.
@@ -691,7 +691,7 @@
     context := makeStorageContext(&TestTransport{Error: cannedError})
     err := context.PutPage(&PutPageRequest{
         Container: "container", Filename: "filename", StartRange: 0,
-        EndRange: 512, Data: nil})
+        EndRange: 511, Data: nil})
     c.Assert(err, NotNil)
 }
 
@@ -702,7 +702,7 @@
     context := makeStorageContext(&TestTransport{Response: response})
     err := context.PutPage(&PutPageRequest{
         Container: "container", Filename: "filename", StartRange: 0,
-        EndRange: 512, Data: nil})
+        EndRange: 511, Data: nil})
     c.Assert(err, NotNil)
     c.Check(err, ErrorMatches, ".*102.*")
     c.Check(err, ErrorMatches, ".*Frotzed.*")
@@ -716,12 +716,21 @@
     context := makeStorageContext(&TestTransport{Response: response})
     err := context.PutPage(&PutPageRequest{
         Container: "container", Filename: "filename", StartRange: 0,
-        EndRange: 512, Data: nil})
+        EndRange: 511, Data: nil})
     serverError, ok := err.(*ServerError)
     c.Check(ok, Equals, true)
     c.Check(serverError.HTTPStatus.StatusCode(), Equals, http.StatusNotFound)
 }
 
+// Range values outside the limits should get rejected.
+func (suite *TestPutPage) TestRangeLimits(c *C) {
+    context := makeStorageContext(&TestTransport{})
+    err := context.PutPage(&PutPageRequest{
+        StartRange: 513, EndRange: 555})
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, ".*StartRange must be a multiple of 512, EndRange must be one less than a multiple of 512.*")
+}
+
 type TestPutBlob struct{}
 
 var _ = Suite(&TestPutBlob{})
@@ -764,6 +773,16 @@
     c.Assert(err, ErrorMatches, "Must supply a size for a page blob")
 }
 
+// PutBlob for a page should return an error when Size is not a multiple
+// of 512 bytes.
+func (suite *TestPutBlob) TestPutPageBlobWithInvalidSiuze(c *C) {
+    context := makeStorageContext(&TestTransport{})
+    err := context.PutBlob(&PutBlobRequest{
+        Container: "container", BlobType: "page", Filename: "blob",
+        Size: 1015})
+    c.Assert(err, ErrorMatches, "Size must be a multiple of 512 bytes")
+}
+
 // Passing a BlobType other than page or block results in a panic.
 func (suite *TestPutBlob) TestBlobType(c *C) {
     defer func() {