Code review comment for lp:~julian-edwards/gwacl/vhd-define

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good, but I'm concerned about [3]. I'm not 100% sure that's a
bug, so Needs Information.

[1]

+    Size           int       // How many bytes from the Filesystem data to
+                             // upload.  *Must* be a modulo of 512.

I'm not sure modulo is the right word here (and I don't think it's a
noun either). How about s/modulo/multiple/?

[2]

+    if err != nil {
+        // This really shouldn't ever happen since there's a test to make sure
+        // it can be decoded.
+        panic("Unable to base64 decode VHD_FOOTER")
+    }

We lose the original error here. How about panic(err) instead?

[3]

+    err = context.PutPage(&PutPageRequest{
+        Container:  req.Container,
+        Filename:   req.Filename,
+        StartRange: VHD_SIZE-513,
+        EndRange:   VHD_SIZE-1,
+        Data:       dataReader,
+    })

This should probably use len(data) instead of literals.

Also, this means that the maximum size of the filesystem data is
VHD_SIZE - len(data), not VHD_SIZE.

[4]

+func (context *StorageContext) CreateVHD(req *CreateVHDRequest) error {

This isn't a general purpose function for creating a VHD, so I think
it ought to be renamed to, say, CreateVHDForUserData or something like
that.

review: Needs Information

« Back to merge proposal