Merge lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

state;apiserver: Fix a race - lp bug #1067979

This introduces some changes to how charm store
charms are added through the API (in state and
to provider storage). Now PrepareStoreCharmUpload
is called before trying to download the charm,
repackage it and upload it to storage, in order
to reserve a charm URL in state with pending
state. Added a test that demonstrates multiple
concurrent deployments of the same charm does
not cause the race issues, like mentioned in
the bug.

A few drive-by fixes brought up during review:
* Added ReadSHA256 and ReadFileSHA256 helpers in
  utils, and changed most places where hashes
  are calculated to use them.
* Charms are now uploaded to storage with a
  randomly generated archive names with the
  format "<charm-name>-<revision>-<uuid>".
  This allows multiple concurrent uploads
  to happen safely, and at the end AddCharm
  in the API checks to see if the charm info
  is already updated in state and if so, deletes
  the duplicated upload.
* Added GetEnvironStorage helper to environs/testing.
* Fixed potential compatibility issues with older
  versions and the recently added PendingUpload
  and Placeholder fields of the charm document.

Also tested multiple concurrent deployments with
the local provider manually and updated the bug
accordingly.

https://codereview.appspot.com/53210044/

R=fwereade

state;apiserver: Fix a race - lp bug #1067979

This introduces some changes to how charm store
charms are added through the API (in state and
to provider storage). Now PrepareStoreCharmUpload
is called before trying to download the charm,
repackage it and upload it to storage, in order
to reserve a charm URL in state with pending
state. Added a test that demonstrates multiple
concurrent deployments of the same charm does
not cause the race issues, like mentioned in
the bug.

A few drive-by fixes brought up during review:
* Added ReadSHA256 and ReadFileSHA256 helpers in
  utils, and changed most places where hashes
  are calculated to use them.
* Charms are now uploaded to storage with a
  randomly generated archive names with the
  format "<charm-name>-<revision>-<uuid>".
  This allows multiple concurrent uploads
  to happen safely, and at the end AddCharm
  in the API checks to see if the charm info
  is already updated in state and if so, deletes
  the duplicated upload.
* Added GetEnvironStorage helper to environs/testing.
* Fixed potential compatibility issues with older
  versions and the recently added PendingUpload
  and Placeholder fields of the charm document.

Also tested multiple concurrent deployments with
the local provider manually and updated the bug
accordingly.

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+203291_code.launchpad.net,

Message:
Please take a look.

Description:
state;apiserver: Fix a race - lp bug #1067979

This introduces some changes to how charm store
charms are added through the API (in state and
to provider storage). Now PrepareStoreCharmUpload
is called before trying to download the charm,
repackage it and upload it to storage, in order
to reserve a charm URL in state with pending
state. Added a test that demonstrates multiple
concurrent deployments of the same charm does
not cause the race issues, like mentioned in
the bug.

Also tested manually with the local provider
and updated the bug accordingly.

https://code.launchpad.net/~dimitern/juju-core/260-lp-1067979-duplicate-charm/+merge/203291

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/53210044/

Affected files (+236, -54 lines):
   A [revision details]
   M state/apiserver/charms_test.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go
   M state/state.go
   M state/state_test.go

[William Reade (fwereade)]

A few comments, reprising what we discussed at lunch

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client.go#newcode885
state/apiserver/client/client.go:885: storageURL, err :=
storage.URL(name)
I'd prefer to generate unique names than to potentially have multiple
writers to the same storage location. I think it'd be best to have
multiple independent downloads, and multiple independent uploads, and
then just to have a single worker win the race and set a URL that
definitely worked in state. No need to rename or anything -- as far as
I'm aware, charm URL is totally independent of storage URL anyway, and
if it's not that's a bug that should be fixed anyway ;).

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_test.go#newcode1850
state/apiserver/client/client_test.go:1850: }
in light of the previous comment,we *do* want multiple charms uploaded
(but all except one to be deleted). And whichever one isn't deleted
should match the archive url of the charm in state.

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client.go#newcode885
state/apiserver/client/client.go:885: storageURL, err :=
storage.URL(name)
On 2014/01/27 12:46:16, fwereade wrote:
> I'd prefer to generate unique names than to potentially have multiple
writers to
> the same storage location. I think it'd be best to have multiple
independent
> downloads, and multiple independent uploads, and then just to have a
single
> worker win the race and set a URL that definitely worked in state. No
need to
> rename or anything -- as far as I'm aware, charm URL is totally
independent of
> storage URL anyway, and if it's not that's a bug that should be fixed
anyway ;).

I'll add code to generate a storage URL based on a random UUID string.
If you don't mind, I'll also follow John's suggestion to include the
charm name and revision as prefix, to make the string a bit less
cryptic.

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/1/state/apiserver/client/client_test.go#newcode1850
state/apiserver/client/client_test.go:1850: }
On 2014/01/27 12:46:16, fwereade wrote:
> in light of the previous comment,we *do* want multiple charms uploaded
(but all
> except one to be deleted). And whichever one isn't deleted should
match the
> archive url of the charm in state.

Will change accordingly.

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/

[William Reade (fwereade)]

few more comments, but the heart of it looks solid

https://codereview.appspot.com/53210044/diff/40001/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/53210044/diff/40001/charm/url.go#newcode261
charm/url.go:261: func (u *URL) ArchiveName() (string, error) {
I'm certain this is not part of the charm package -- np with the code,
but it's very definitely an internal detail of the state server.

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go#newcode243
state/apiserver/charms.go:243: func GetSHA256(source io.Reader) (string,
int64, error) {
whereas this feels maybe utilsy -- and we do this *all over the place*,
so please hunt them down and make them all use the same utils func,
assuming that is practical

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client.go#newcode908
state/apiserver/client/client.go:908: return fmt.Errorf("cannot remove
duplicated charm from storage: %v", err)
log it, I think, but don't spoil the user's day -- we still managed to
do what she asked, after all

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go#newcode1821
state/apiserver/client/client_test.go:1821: close(ops)
race? we should expect 9 Deletes as well, I think

https://codereview.appspot.com/53210044/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode606
state/state.go:606: err = st.charms.Find(D{{"_id", curl},
{"placeholder", false}}).One(&uploadedCharm)
placeholder might be false in a new installation, or missing in an old
one

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode624
state/state.go:624: Insert: uploadedCharm,
if there *was* a placeholder charm, the insert would fail, wouldn't it?

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode642
state/state.go:642: StillPlaceholder = D{{"placeholder", true}}
I think these should probably not be exported

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/diff/40001/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/53210044/diff/40001/charm/url.go#newcode261
charm/url.go:261: func (u *URL) ArchiveName() (string, error) {
On 2014/01/28 08:19:41, fwereade wrote:
> I'm certain this is not part of the charm package -- np with the code,
but it's
> very definitely an internal detail of the state server.

I'll move it to the state package then.

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/charms.go#newcode243
state/apiserver/charms.go:243: func GetSHA256(source io.Reader) (string,
int64, error) {
On 2014/01/28 08:19:41, fwereade wrote:
> whereas this feels maybe utilsy -- and we do this *all over the
place*, so
> please hunt them down and make them all use the same utils func,
assuming that
> is practical

Done.

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client.go#newcode908
state/apiserver/client/client.go:908: return fmt.Errorf("cannot remove
duplicated charm from storage: %v", err)
On 2014/01/28 08:19:41, fwereade wrote:
> log it, I think, but don't spoil the user's day -- we still managed to
do what
> she asked, after all

Wasn't sure whether to log an error or warning. I'll just log it as an
error then, because it seems serious enough (it shouldn't happen
usually).

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go#newcode1821
state/apiserver/client/client_test.go:1821: close(ops)
On 2014/01/28 08:19:41, fwereade wrote:
> race? we should expect 9 Deletes as well, I think

There is no way to detect storage removals with the dummy provider I
could find. But I am checking below that we do have a single upload at
the end. Not sure what race do you mean?

https://codereview.appspot.com/53210044/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode606
state/state.go:606: err = st.charms.Find(D{{"_id", curl},
{"placeholder", false}}).One(&uploadedCharm)
On 2014/01/28 08:19:41, fwereade wrote:
> placeholder might be false in a new installation, or missing in an old
one

I'll modify the condition, but this applies to other places in the code
as well.

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode624
state/state.go:624: Insert: uploadedCharm,
On 2014/01/28 08:19:41, fwereade wrote:
> if there *was* a placeholder charm, the insert would fail, wouldn't
it?

Right, but since I changed how we select the document above it shouldn't
be a problem.

https://codereview.appspot.com/53210044/diff/40001/state/state.go#newcode642
state/state.go:642: StillPlaceholder = D{{"placeholder", true}}
On 2014/01/28 08:19:41, fwereade wrote:
> I think these should pr...

[William Reade (fwereade)]

Very nice, last round I think

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/40001/state/apiserver/client/client_test.go#newcode1821
state/apiserver/client/client_test.go:1821: close(ops)
On 2014/01/28 10:55:05, dimitern wrote:
> On 2014/01/28 08:19:41, fwereade wrote:
> > race? we should expect 9 Deletes as well, I think

> There is no way to detect storage removals with the dummy provider I
could find.
> But I am checking below that we do have a single upload at the end.
Not sure
> what race do you mean?

I was worried that the deletes *were* pushing ops, and that channel
might sometimes be closed before the ops stopped. But yeah, looks like
no delete ops; and I'm fine with the test. Just seemed a bit weird to
mix the testing styles -- but I think we're good as is. Thanks.

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go#newcode229
state/apiserver/charms.go:229: func GetEnvironStorage(st *state.State)
(storage.Storage, error) {
maybe put this in export_test rather than growing the interface here?

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/client_test.go#newcode1828
state/apiserver/client/client_test.go:1828: storage, err :=
apiserver.GetEnvironStorage(s.State)
ah, ok, you can't put it in export_test. Hmm. I think I'd maybe prefer
it if this were accessed via the environ on the test, but I'm not sure
of the tradeoffs -- I'll leave it to your judgment.

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode480
state/state.go:480: what := D{{"_id", curl}, {"placeholder", false},
{"pendingupload", false}}
Yeah, these bits do need to be fixed. Sorry, but they *will* break
stuff.

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
Don't we still need to deal with a placeholder here?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go
File utils/trivial.go (right):

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode127
utils/trivial.go:127: func GetSHA256(source io.Reader) (string, int64,
error) {
ReadSHA256?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode139
utils/trivial.go:139: func GetFileSHA256(filename string) (string,
int64, error) {
ReadFileSHA256?

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode146
utils/trivial.go:146: }
Also, please test these. Not quite trivial enough to be skippable IMO.

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/charms.go#newcode229
state/apiserver/charms.go:229: func GetEnvironStorage(st *state.State)
(storage.Storage, error) {
On 2014/01/28 12:00:32, fwereade wrote:
> maybe put this in export_test rather than growing the interface here?

It was there, but I needed it in two places. I'll put it in
environs/testing.

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/apiserver/client/client_test.go#newcode1828
state/apiserver/client/client_test.go:1828: storage, err :=
apiserver.GetEnvironStorage(s.State)
On 2014/01/28 12:00:32, fwereade wrote:
> ah, ok, you can't put it in export_test. Hmm. I think I'd maybe prefer
it if
> this were accessed via the environ on the test, but I'm not sure of
the
> tradeoffs -- I'll leave it to your judgment.

I'll put it in environs/testing.

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode480
state/state.go:480: what := D{{"_id", curl}, {"placeholder", false},
{"pendingupload", false}}
On 2014/01/28 12:00:32, fwereade wrote:
> Yeah, these bits do need to be fixed. Sorry, but they *will* break
stuff.

Done.

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
On 2014/01/28 12:00:32, fwereade wrote:
> Don't we still need to deal with a placeholder here?

If we reach this point we know it's not a placeholder and there's no
document for this charm URL. Not setting Placeholder: false here works,
because it's the default value. So it's OK I think.

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go
File utils/trivial.go (right):

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode127
utils/trivial.go:127: func GetSHA256(source io.Reader) (string, int64,
error) {
On 2014/01/28 12:00:32, fwereade wrote:
> ReadSHA256?

Done.

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode139
utils/trivial.go:139: func GetFileSHA256(filename string) (string,
int64, error) {
On 2014/01/28 12:00:32, fwereade wrote:
> ReadFileSHA256?

Done.

https://codereview.appspot.com/53210044/diff/60001/utils/trivial.go#newcode146
utils/trivial.go:146: }
On 2014/01/28 12:00:32, fwereade wrote:
> Also, please test these. Not quite trivial enough to be skippable IMO.

Done.

https://codereview.appspot.com/53210044/

[William Reade (fwereade)]

I still think there's something up with PrepareStoreCharmUpload, but
it's getting late and I may be being dense. Ping me tomorrow morning?

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
On 2014/01/28 17:15:02, dimitern wrote:
> On 2014/01/28 12:00:32, fwereade wrote:
> > Don't we still need to deal with a placeholder here?

> If we reach this point we know it's not a placeholder and there's no
document
> for this charm URL. Not setting Placeholder: false here works, because
it's the
> default value. So it's OK I think.

We will still have fallen through the block above if err is nil but
placeholder is true. Right? I had to think about this harder than I'd
like, which might indicate that we could make that block clearer, but
nothing really springs to mind very obviously... perhaps a switch?

ops := []txn.Op{}
switch err {
case mgo.ErrNotFound:
     ops = append(ops, <insert op>)
case nil:
     if uploadedCharm.Placeholder {
         ops = append(ops, <update op>)
     } else {
         return newCharm(st, &uploadedCharm)
     }
default:
     return nil, err
}

..? Maybe not very compelling?

This might be a case where really pedantic SetTransactionHooks-style
testing would be a boon -- even if the logic is hard to follow we can
just churn through the possible test cases and have some more certainty.

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/60001/state/state.go#newcode625
state/state.go:625: Insert: uploadedCharm,
On 2014/01/28 23:09:31, fwereade wrote:
> On 2014/01/28 17:15:02, dimitern wrote:
> > On 2014/01/28 12:00:32, fwereade wrote:
> > > Don't we still need to deal with a placeholder here?
> >
> > If we reach this point we know it's not a placeholder and there's no
document
> > for this charm URL. Not setting Placeholder: false here works,
because it's
> the
> > default value. So it's OK I think.

> We will still have fallen through the block above if err is nil but
placeholder
> is true. Right? I had to think about this harder than I'd like, which
might
> indicate that we could make that block clearer, but nothing really
springs to
> mind very obviously... perhaps a switch?

> ops := []txn.Op{}
> switch err {
> case mgo.ErrNotFound:
> ops = append(ops, <insert op>)
> case nil:
> if uploadedCharm.Placeholder {
> ops = append(ops, <update op>)
> } else {
> return newCharm(st, &uploadedCharm)
> }
> default:
> return nil, err
> }

> ..? Maybe not very compelling?

> This might be a case where really pedantic SetTransactionHooks-style
testing
> would be a boon -- even if the logic is hard to follow we can just
churn through
> the possible test cases and have some more certainty.

I added a specific test case to cover "uploading a store charm when a
placeholder exists", and indeed it uncovered issues, which I fixed now.
Thanks!

As for the transaction hooks, I can't use them unfortunately outside the
state packages (they're all defined in export_test). But despite that, I
think the code looks solid now.

https://codereview.appspot.com/53210044/

[William Reade (fwereade)]

Most of these are genuinely trivial now -- if we're in agreement on them
all, LGTM, but ping me if there's any uncertainty

https://codereview.appspot.com/53210044/diff/100001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/charm.go#newcode87
state/charm.go:87: func CharmArchiveName(name string, revision int)
(string, error) {
quibble quibble whine, I think this probably belongs in apiserver/client

https://codereview.appspot.com/53210044/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode633
state/state.go:633: Assert: txn.DocExists,
hmm, should be be asserting that it hasn't had archiveurl (etc) filled
in? I think there's a very very cornery corner case in which we read
while it's still a placeholder, but write when it's already uploaded.

That'd be a good transaction-hooks test ;).

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode661
state/state.go:661: if err != nil {
this makes me twitch a little -- AFAICT the only possible value is
txn.ErrAborted, but that assumption feels very vulnerable to code
perturbations in the loop above. Maybe check for err == nil inside the
loop, and return newCharm() there, so that ErrAborted becomes the *only*
way to fall out of the loop, and just always return
ErrExcessiveContention?

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go
File utils/trivial_test.go (right):

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#newcode87
utils/trivial_test.go:87: {
}{{
     ...
},{
     ...

to compress both vertical and horizontal space?

And, hey, can we please just move these inside the test func? If they're
not used elsewhere they really don't deserve to be global IMO.

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#newcode105
utils/trivial_test.go:105: func (utilsSuite)
TestReadSHA256AndReadFileSHA256(c *gc.C) {
*utilsSuite, please, across the board, lest someone add methods that
require pointer receivers and silently deactivate all the other tests.

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/diff/100001/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/charm.go#newcode87
state/charm.go:87: func CharmArchiveName(name string, revision int)
(string, error) {
On 2014/01/29 16:01:22, fwereade wrote:
> quibble quibble whine, I think this probably belongs in
apiserver/client

Done.

https://codereview.appspot.com/53210044/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode633
state/state.go:633: Assert: txn.DocExists,
On 2014/01/29 16:01:22, fwereade wrote:
> hmm, should be be asserting that it hasn't had archiveurl (etc) filled
in? I
> think there's a very very cornery corner case in which we read while
it's still
> a placeholder, but write when it's already uploaded.

> That'd be a good transaction-hooks test ;).

Done.

https://codereview.appspot.com/53210044/diff/100001/state/state.go#newcode661
state/state.go:661: if err != nil {
On 2014/01/29 16:01:22, fwereade wrote:
> this makes me twitch a little -- AFAICT the only possible value is
> txn.ErrAborted, but that assumption feels very vulnerable to code
perturbations
> in the loop above. Maybe check for err == nil inside the loop, and
return
> newCharm() there, so that ErrAborted becomes the *only* way to fall
out of the
> loop, and just always return ErrExcessiveContention?

Done.

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go
File utils/trivial_test.go (right):

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#newcode87
utils/trivial_test.go:87: {
On 2014/01/29 16:01:22, fwereade wrote:
> }{{
> ...
> },{
> ...

> to compress both vertical and horizontal space?

> And, hey, can we please just move these inside the test func? If
they're not
> used elsewhere they really don't deserve to be global IMO.

Done.

https://codereview.appspot.com/53210044/diff/100001/utils/trivial_test.go#newcode105
utils/trivial_test.go:105: func (utilsSuite)
TestReadSHA256AndReadFileSHA256(c *gc.C) {
On 2014/01/29 16:01:22, fwereade wrote:
> *utilsSuite, please, across the board, lest someone add methods that
require
> pointer receivers and silently deactivate all the other tests.

Done.

https://codereview.appspot.com/53210044/

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.291s
ok launchpad.net/juju-core/agent/tools 0.305s
ok launchpad.net/juju-core/bzr 7.397s
ok launchpad.net/juju-core/cert 3.282s
ok launchpad.net/juju-core/charm 0.605s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.105s
ok launchpad.net/juju-core/cmd 0.247s
ok launchpad.net/juju-core/cmd/charm-admin 0.867s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 277.574s
ok launchpad.net/juju-core/cmd/jujud 62.207s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.802s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.037s
ok launchpad.net/juju-core/container/factory 0.052s
ok launchpad.net/juju-core/container/kvm 0.249s
ok launchpad.net/juju-core/container/kvm/mock 0.036s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.422s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.263s
ok launchpad.net/juju-core/environs 3.143s
ok launchpad.net/juju-core/environs/bootstrap 5.124s
ok launchpad.net/juju-core/environs/cloudinit 0.736s
ok launchpad.net/juju-core/environs/config 3.309s
ok launchpad.net/juju-core/environs/configstore 0.056s
ok launchpad.net/juju-core/environs/filestorage 0.035s
ok launchpad.net/juju-core/environs/httpstorage 0.864s
ok launchpad.net/juju-core/environs/imagemetadata 0.652s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.063s
ok launchpad.net/juju-core/environs/jujutest 0.216s
ok launchpad.net/juju-core/environs/manual 8.488s
ok launchpad.net/juju-core/environs/simplestreams 0.401s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.387s
ok launchpad.net/juju-core/environs/storage 1.199s
ok launchpad.net/juju-core/environs/sync 35.792s
ok launchpad.net/juju-core/environs/testing 0.217s
ok launchpad.net/juju-core/environs/tools 6.906s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 24.611s
ok launchpad.net/juju-core/juju/osenv 0.021s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.025s
ok launchpad.net/juju-core/names 0.028s
? launchpad....

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.855s
ok launchpad.net/juju-core/agent/tools 0.243s
ok launchpad.net/juju-core/bzr 7.603s
ok launchpad.net/juju-core/cert 3.491s
ok launchpad.net/juju-core/charm 0.600s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.085s
ok launchpad.net/juju-core/cmd 0.213s
ok launchpad.net/juju-core/cmd/charm-admin 0.864s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 238.171s
ok launchpad.net/juju-core/cmd/jujud 62.620s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 11.807s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.030s
ok launchpad.net/juju-core/container 0.037s
ok launchpad.net/juju-core/container/factory 0.052s
ok launchpad.net/juju-core/container/kvm 0.295s
ok launchpad.net/juju-core/container/kvm/mock 0.052s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.445s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.387s
ok launchpad.net/juju-core/environs 3.076s
ok launchpad.net/juju-core/environs/bootstrap 4.712s
ok launchpad.net/juju-core/environs/cloudinit 0.631s
ok launchpad.net/juju-core/environs/config 3.180s
ok launchpad.net/juju-core/environs/configstore 0.040s
ok launchpad.net/juju-core/environs/filestorage 0.034s
ok launchpad.net/juju-core/environs/httpstorage 0.956s
ok launchpad.net/juju-core/environs/imagemetadata 0.601s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.061s
ok launchpad.net/juju-core/environs/jujutest 0.229s
ok launchpad.net/juju-core/environs/manual 9.763s
ok launchpad.net/juju-core/environs/simplestreams 0.367s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.223s
ok launchpad.net/juju-core/environs/storage 1.177s
ok launchpad.net/juju-core/environs/sync 34.614s
ok launchpad.net/juju-core/environs/testing 0.214s
ok launchpad.net/juju-core/environs/tools 6.831s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.024s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 24.499s
ok launchpad.net/juju-core/juju/osenv 0.022s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.025s
ok launchpad.net/juju-core/names 0.027s
? launchpad...

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.313s
ok launchpad.net/juju-core/agent/tools 0.253s
ok launchpad.net/juju-core/bzr 7.768s
ok launchpad.net/juju-core/cert 3.751s
ok launchpad.net/juju-core/charm 0.598s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.032s
ok launchpad.net/juju-core/cloudinit/sshinit 1.137s
ok launchpad.net/juju-core/cmd 0.243s
ok launchpad.net/juju-core/cmd/charm-admin 0.817s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 258.730s
ok launchpad.net/juju-core/cmd/jujud 62.132s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.465s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.038s
ok launchpad.net/juju-core/container/factory 0.055s
ok launchpad.net/juju-core/container/kvm 0.342s
ok launchpad.net/juju-core/container/kvm/mock 0.056s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.421s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.320s
ok launchpad.net/juju-core/environs 3.263s
ok launchpad.net/juju-core/environs/bootstrap 4.696s
ok launchpad.net/juju-core/environs/cloudinit 0.676s
ok launchpad.net/juju-core/environs/config 4.070s
ok launchpad.net/juju-core/environs/configstore 0.080s
ok launchpad.net/juju-core/environs/filestorage 0.034s
ok launchpad.net/juju-core/environs/httpstorage 0.908s
ok launchpad.net/juju-core/environs/imagemetadata 0.695s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.063s
ok launchpad.net/juju-core/environs/jujutest 0.257s
ok launchpad.net/juju-core/environs/manual 9.759s
ok launchpad.net/juju-core/environs/simplestreams 0.355s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.335s
ok launchpad.net/juju-core/environs/storage 1.165s
ok launchpad.net/juju-core/environs/sync 35.200s
ok launchpad.net/juju-core/environs/testing 0.209s
ok launchpad.net/juju-core/environs/tools 6.839s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.024s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 21.534s
ok launchpad.net/juju-core/juju/osenv 0.020s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.022s
ok launchpad.net/juju-core/names 0.027s
? launchpad....

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.241s
ok launchpad.net/juju-core/agent/tools 0.265s
ok launchpad.net/juju-core/bzr 7.509s
ok launchpad.net/juju-core/cert 3.259s
ok launchpad.net/juju-core/charm 0.659s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.063s
ok launchpad.net/juju-core/cmd 0.231s
ok launchpad.net/juju-core/cmd/charm-admin 0.827s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 257.042s
ok launchpad.net/juju-core/cmd/jujud 63.020s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 13.084s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.029s
ok launchpad.net/juju-core/container 0.043s
ok launchpad.net/juju-core/container/factory 0.074s
ok launchpad.net/juju-core/container/kvm 0.257s
ok launchpad.net/juju-core/container/kvm/mock 0.036s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.363s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.281s
ok launchpad.net/juju-core/environs 3.259s
ok launchpad.net/juju-core/environs/bootstrap 4.443s
ok launchpad.net/juju-core/environs/cloudinit 0.672s
ok launchpad.net/juju-core/environs/config 2.722s
ok launchpad.net/juju-core/environs/configstore 0.049s
ok launchpad.net/juju-core/environs/filestorage 0.032s
ok launchpad.net/juju-core/environs/httpstorage 1.023s
ok launchpad.net/juju-core/environs/imagemetadata 0.685s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.070s
ok launchpad.net/juju-core/environs/jujutest 0.249s
ok launchpad.net/juju-core/environs/manual 8.911s
ok launchpad.net/juju-core/environs/simplestreams 0.359s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.247s
ok launchpad.net/juju-core/environs/storage 1.211s
ok launchpad.net/juju-core/environs/sync 34.656s
ok launchpad.net/juju-core/environs/testing 0.203s
ok launchpad.net/juju-core/environs/tools 6.884s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 25.459s
ok launchpad.net/juju-core/juju/osenv 0.020s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.035s
ok launchpad.net/juju-core/names 0.027s
? launchpad...

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/

[Dimiter Naydenov (dimitern)]

Self-approving after fixing uniter tests.

review: Approve

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.293s
ok launchpad.net/juju-core/agent/tools 0.253s
ok launchpad.net/juju-core/bzr 7.562s
ok launchpad.net/juju-core/cert 3.083s
ok launchpad.net/juju-core/charm 0.669s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.032s
ok launchpad.net/juju-core/cloudinit/sshinit 1.096s
ok launchpad.net/juju-core/cmd 0.279s
ok launchpad.net/juju-core/cmd/charm-admin 0.867s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 253.577s
*** Test killed: ran too long (10m0s).
FAIL launchpad.net/juju-core/cmd/jujud 600.018s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 13.476s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.048s
ok launchpad.net/juju-core/container 0.055s
ok launchpad.net/juju-core/container/factory 0.061s
ok launchpad.net/juju-core/container/kvm 0.307s
ok launchpad.net/juju-core/container/kvm/mock 0.048s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.384s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.287s
ok launchpad.net/juju-core/environs 3.200s
ok launchpad.net/juju-core/environs/bootstrap 4.596s
ok launchpad.net/juju-core/environs/cloudinit 0.660s
ok launchpad.net/juju-core/environs/config 2.831s
ok launchpad.net/juju-core/environs/configstore 0.062s
ok launchpad.net/juju-core/environs/filestorage 0.040s
ok launchpad.net/juju-core/environs/httpstorage 1.054s
ok launchpad.net/juju-core/environs/imagemetadata 0.666s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.080s
ok launchpad.net/juju-core/environs/jujutest 0.303s
ok launchpad.net/juju-core/environs/manual 9.640s
ok launchpad.net/juju-core/environs/simplestreams 0.380s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 2.166s
ok launchpad.net/juju-core/environs/storage 1.244s
ok launchpad.net/juju-core/environs/sync 34.538s
ok launchpad.net/juju-core/environs/testing 0.243s
ok launchpad.net/juju-core/environs/tools 7.067s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.025s
ok launchpad.net/juju-core/instance 0.035s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 27.392s
ok launchpad.net/juju-core/juju/osenv 0.040s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.029s
ok launchpad.net/juju-core/log/syslog 0.027s
ok launchpad.n...

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/53210044/

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/260-lp-1067979-duplicate-charm into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.188s
ok launchpad.net/juju-core/agent/tools 0.257s
ok launchpad.net/juju-core/bzr 7.914s
ok launchpad.net/juju-core/cert 3.140s
ok launchpad.net/juju-core/charm 0.642s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.071s
ok launchpad.net/juju-core/cmd 0.222s
ok launchpad.net/juju-core/cmd/charm-admin 0.865s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 250.766s
ok launchpad.net/juju-core/cmd/jujud 63.331s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 12.447s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.029s
ok launchpad.net/juju-core/container 0.038s
ok launchpad.net/juju-core/container/factory 0.065s
ok launchpad.net/juju-core/container/kvm 0.354s
ok launchpad.net/juju-core/container/kvm/mock 0.055s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.314s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.287s
ok launchpad.net/juju-core/environs 3.346s
ok launchpad.net/juju-core/environs/bootstrap 5.054s
ok launchpad.net/juju-core/environs/cloudinit 0.684s
ok launchpad.net/juju-core/environs/config 5.756s
ok launchpad.net/juju-core/environs/configstore 0.068s
ok launchpad.net/juju-core/environs/filestorage 0.034s
ok launchpad.net/juju-core/environs/httpstorage 1.023s
ok launchpad.net/juju-core/environs/imagemetadata 0.799s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.063s
ok launchpad.net/juju-core/environs/jujutest 0.339s
ok launchpad.net/juju-core/environs/manual 7.733s
ok launchpad.net/juju-core/environs/simplestreams 0.331s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.588s
ok launchpad.net/juju-core/environs/storage 1.217s
ok launchpad.net/juju-core/environs/sync 36.899s
ok launchpad.net/juju-core/environs/testing 0.201s
ok launchpad.net/juju-core/environs/tools 7.064s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.017s
ok launchpad.net/juju-core/instance 0.024s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 30.207s
ok launchpad.net/juju-core/juju/osenv 0.021s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.029s
ok launchpad.net/juju-core/names 0.030s
? launchpad...

=== modified file 'charm/repo.go'
--- charm/repo.go	2014-01-22 16:48:19 +0000
+++ charm/repo.go	2014-01-30 14:23:21 +0000
@@ -4,8 +4,6 @@
 package charm
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -299,16 +297,11 @@
 // verify returns an error unless a file exists at path with a hex-encoded
 // SHA256 matching digest.
 func verify(path, digest string) error {
-	f, err := os.Open(path)
+	hash, _, err := utils.ReadFileSHA256(path)
 	if err != nil {
 		return err
 	}
-	defer f.Close()
-	h := sha256.New()
-	if _, err := io.Copy(h, f); err != nil {
-		return err
-	}
-	if hex.EncodeToString(h.Sum(nil)) != digest {
+	if hash != digest {
 		return fmt.Errorf("bad SHA256 of %q", path)
 	}
 	return nil

=== modified file 'charm/testing/mockstore.go'
--- charm/testing/mockstore.go	2014-01-28 04:58:43 +0000
+++ charm/testing/mockstore.go	2014-01-30 14:23:21 +0000
@@ -4,12 +4,12 @@
 package testing
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
+	"bytes"
 	"encoding/json"
-	"io/ioutil"
+	"io"
 	"net"
 	"net/http"
+	"os"
 	"strconv"
 	"strings"
 
@@ -18,6 +18,7 @@
 
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/utils"
 )
 
 var logger = loggo.GetLogger("juju.charm.testing.mockstore")
@@ -38,12 +39,14 @@
 // NewMockStore creates a mock charm store containing the specified charms.
 func NewMockStore(c *gc.C, charms map[string]int) *MockStore {
 	s := &MockStore{charms: charms}
-	bytes, err := ioutil.ReadFile(testing.Charms.BundlePath(c.MkDir(), "dummy"))
-	c.Assert(err, gc.IsNil)
-	s.bundleBytes = bytes
-	h := sha256.New()
-	h.Write(bytes)
-	s.bundleSha256 = hex.EncodeToString(h.Sum(nil))
+	f, err := os.Open(testing.Charms.BundlePath(c.MkDir(), "dummy"))
+	c.Assert(err, gc.IsNil)
+	defer f.Close()
+	buf := &bytes.Buffer{}
+	s.bundleSha256, _, err = utils.ReadSHA256(io.TeeReader(f, buf))
+	c.Assert(err, gc.IsNil)
+	s.bundleBytes = buf.Bytes()
+	c.Assert(err, gc.IsNil)
 	s.mux = http.NewServeMux()
 	s.mux.HandleFunc("/charm-info", s.serveInfo)
 	s.mux.HandleFunc("/charm-event", s.serveEvent)

=== modified file 'environs/sync/sync.go'
--- environs/sync/sync.go	2014-01-30 06:21:03 +0000
+++ environs/sync/sync.go	2014-01-30 14:23:21 +0000
@@ -5,7 +5,6 @@
 
 import (
 	"bytes"
-	"crypto/sha256"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -166,22 +165,17 @@
 	if err != nil {
 		return err
 	}
+	buf := &bytes.Buffer{}
 	srcFile := resp.Body
 	defer srcFile.Close()
-	// We have to buffer the content, because Put requires the content
-	// length, but Get only returns us a ReadCloser
-	buf := &bytes.Buffer{}
-	nBytes, err := io.Copy(buf, srcFile)
+	tool.SHA256, tool.Size, err = utils.ReadSHA256(io.TeeReader(srcFile, buf))
 	if err != nil {
 		return err
 	}
-	logger.Infof("downloaded %v (%dkB), uploading", toolsName, (nBytes+512)/1024)
-	logger.Infof("download %dkB, uploading", (nBytes+512)/1024)
-	sha256hash := sha256.New()
-	sha256hash.Write(buf.Bytes())
-	tool.SHA256 = fmt.Sprintf("%x", sha256hash.Sum(nil))
-	tool.Size = nBytes
-	return dest.Put(toolsName, buf, nBytes)
+	sizeInKB := (tool.Size + 512) / 1024
+	logger.Infof("downloaded %v (%dkB), uploading", toolsName, sizeInKB)
+	logger.Infof("download %dkB, uploading", sizeInKB)
+	return dest.Put(toolsName, buf, tool.Size)
 }
 
 // UploadFunc is the type of Upload, which may be

=== modified file 'environs/testing/storage.go'
--- environs/testing/storage.go	2014-01-14 06:37:55 +0000
+++ environs/testing/storage.go	2014-01-30 14:23:21 +0000
@@ -4,13 +4,16 @@
 package testing
 
 import (
+	"fmt"
 	"io"
 
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/filestorage"
 	"launchpad.net/juju-core/environs/httpstorage"
 	"launchpad.net/juju-core/environs/storage"
+	"launchpad.net/juju-core/state"
 )
 
 // CreateLocalTestStorage returns the listener, which needs to be closed, and
@@ -26,3 +29,17 @@
 	closer = listener
 	return
 }
+
+// GetEnvironStorage creates an Environ from the config in state and
+// returns its storage interface.
+func GetEnvironStorage(st *state.State) (storage.Storage, error) {
+	envConfig, err := st.EnvironConfig()
+	if err != nil {
+		return nil, fmt.Errorf("cannot get environment config: %v", err)
+	}
+	env, err := environs.New(envConfig)
+	if err != nil {
+		return nil, fmt.Errorf("cannot access environment: %v", err)
+	}
+	return env.Storage(), nil
+}

=== modified file 'environs/tools/testing/testing.go'
--- environs/tools/testing/testing.go	2014-01-30 06:21:03 +0000
+++ environs/tools/testing/testing.go	2014-01-30 14:23:21 +0000
@@ -4,9 +4,7 @@
 package testing
 
 import (
-	"crypto/sha256"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"os"
 	"path"
@@ -22,6 +20,7 @@
 	"launchpad.net/juju-core/environs/tools"
 	jc "launchpad.net/juju-core/testing/checkers"
 	coretools "launchpad.net/juju-core/tools"
+	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/utils/set"
 	"launchpad.net/juju-core/version"
 )
@@ -70,13 +69,9 @@
 	if strings.HasPrefix(path, "file://") {
 		path = path[len("file://"):]
 	}
-	f, err := os.Open(path)
-	c.Assert(err, gc.IsNil)
-	defer f.Close()
-	hash := sha256.New()
-	size, err := io.Copy(hash, f)
-	c.Assert(err, gc.IsNil)
-	return size, fmt.Sprintf("%x", hash.Sum(nil))
+	hash, size, err := utils.ReadFileSHA256(path)
+	c.Assert(err, gc.IsNil)
+	return size, hash
 }
 
 // ParseMetadataFromDir loads ToolsMetadata from the specified directory.

=== modified file 'juju/conn.go'
--- juju/conn.go	2014-01-22 22:48:54 +0000
+++ juju/conn.go	2014-01-30 14:23:21 +0000
@@ -4,11 +4,8 @@
 package juju
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
 	stderrors "errors"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net/url"
 	"os"
@@ -223,12 +220,10 @@
 	default:
 		return nil, fmt.Errorf("unknown charm type %T", ch)
 	}
-	h := sha256.New()
-	size, err := io.Copy(h, f)
+	digest, size, err := utils.ReadSHA256(f)
 	if err != nil {
 		return nil, err
 	}
-	digest := hex.EncodeToString(h.Sum(nil))
 	if _, err := f.Seek(0, 0); err != nil {
 		return nil, err
 	}

=== modified file 'juju/conn_test.go'
--- juju/conn_test.go	2014-01-24 12:39:48 +0000
+++ juju/conn_test.go	2014-01-30 14:23:21 +0000
@@ -453,7 +453,7 @@
 
 func (s *DeployLocalSuite) SetUpSuite(c *gc.C) {
 	s.JujuConnSuite.SetUpSuite(c)
-	s.repo = &charm.LocalRepository{Path: coretesting.Charms.Path}
+	s.repo = &charm.LocalRepository{Path: coretesting.Charms.Path()}
 	s.oldCacheDir, charm.CacheDir = charm.CacheDir, c.MkDir()
 }
 

=== modified file 'juju/testing/conn.go'
--- juju/testing/conn.go	2014-01-24 12:39:48 +0000
+++ juju/testing/conn.go	2014-01-30 14:23:21 +0000
@@ -280,7 +280,7 @@
 	ch := testing.Charms.Dir(name)
 	ident := fmt.Sprintf("%s-%d", ch.Meta().Name, ch.Revision())
 	curl := charm.MustParseURL("local:quantal/" + ident)
-	repo, err := charm.InferRepository(curl, testing.Charms.Path)
+	repo, err := charm.InferRepository(curl, testing.Charms.Path())
 	c.Assert(err, gc.IsNil)
 	sch, err := s.Conn.PutCharm(curl, repo, false)
 	c.Assert(err, gc.IsNil)

=== modified file 'juju/testing/repo.go'
--- juju/testing/repo.go	2013-12-19 12:39:49 +0000
+++ juju/testing/repo.go	2014-01-30 14:23:21 +0000
@@ -1,9 +1,6 @@
 package testing
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
-	"io/ioutil"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -13,6 +10,7 @@
 
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/utils"
 )
 
 // RepoSuite acts as a JujuConnSuite but also sets up
@@ -77,11 +75,8 @@
 	resp, err := http.Get(url.String())
 	c.Assert(err, gc.IsNil)
 	defer resp.Body.Close()
-	body, err := ioutil.ReadAll(resp.Body)
+	digest, _, err := utils.ReadSHA256(resp.Body)
 	c.Assert(err, gc.IsNil)
-	h := sha256.New()
-	h.Write(body)
-	digest := hex.EncodeToString(h.Sum(nil))
 	c.Assert(ch.BundleSha256(), gc.Equals, digest)
 }
 

=== modified file 'state/apiserver/charms.go'
--- state/apiserver/charms.go	2013-12-17 09:49:11 +0000
+++ state/apiserver/charms.go	2014-01-30 14:23:21 +0000
@@ -18,8 +18,7 @@
 	"strings"
 
 	"launchpad.net/juju-core/charm"
-	"launchpad.net/juju-core/environs"
-	"launchpad.net/juju-core/environs/storage"
+	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
@@ -199,7 +198,7 @@
 	}
 
 	// Now upload to provider storage.
-	storage, err := getEnvironStorage(h.state)
+	storage, err := envtesting.GetEnvironStorage(h.state)
 	if err != nil {
 		return fmt.Errorf("cannot access provider storage: %v", err)
 	}
@@ -223,17 +222,3 @@
 	}
 	return nil
 }
-
-// getEnvironStorage creates an Environ from the config in state and
-// returns its storage interface.
-func getEnvironStorage(st *state.State) (storage.Storage, error) {
-	envConfig, err := st.EnvironConfig()
-	if err != nil {
-		return nil, fmt.Errorf("cannot get environment config: %v", err)
-	}
-	env, err := environs.New(envConfig)
-	if err != nil {
-		return nil, fmt.Errorf("cannot access environment: %v", err)
-	}
-	return env.Storage(), nil
-}

=== modified file 'state/apiserver/charms_test.go'
--- state/apiserver/charms_test.go	2013-12-17 09:49:11 +0000
+++ state/apiserver/charms_test.go	2014-01-30 14:23:21 +0000
@@ -4,8 +4,6 @@
 package apiserver_test
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -17,10 +15,10 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/charm"
+	envtesting "launchpad.net/juju-core/environs/testing"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
-	"launchpad.net/juju-core/state/apiserver"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/utils"
@@ -131,7 +129,7 @@
 	c.Assert(sch.Revision(), gc.Equals, 2)
 	c.Assert(sch.IsUploaded(), jc.IsTrue)
 	// No more checks for these two here, because they
-	// are verified in TestUploadRequiresSingleUploadedFile.
+	// are verified in TestUploadRespectsLocalRevision.
 	c.Assert(sch.BundleURL(), gc.Not(gc.Equals), "")
 	c.Assert(sch.BundleSha256(), gc.Not(gc.Equals), "")
 }
@@ -164,16 +162,23 @@
 	c.Assert(err, gc.IsNil)
 
 	// Finally, verify the SHA256 and uploaded URL.
-	expectedSHA256, _, err := getSHA256(tempFile)
+	expectedSHA256, _, err := utils.ReadSHA256(tempFile)
 	c.Assert(err, gc.IsNil)
 	name := charm.Quote(expectedURL.String())
-	storage, err := apiserver.GetEnvironStorage(s.State)
+	storage, err := envtesting.GetEnvironStorage(s.State)
 	c.Assert(err, gc.IsNil)
 	expectedUploadURL, err := storage.URL(name)
 	c.Assert(err, gc.IsNil)
 
 	c.Assert(sch.BundleURL().String(), gc.Equals, expectedUploadURL)
 	c.Assert(sch.BundleSha256(), gc.Equals, expectedSHA256)
+
+	reader, err := storage.Get(name)
+	c.Assert(err, gc.IsNil)
+	defer reader.Close()
+	downloadedSHA256, _, err := utils.ReadSHA256(reader)
+	c.Assert(err, gc.IsNil)
+	c.Assert(downloadedSHA256, gc.Equals, expectedSHA256)
 }
 
 func (s *charmsSuite) charmsURI(c *gc.C, query string) string {
@@ -230,13 +235,3 @@
 	}
 	c.Check(resp.StatusCode, gc.Equals, expCode)
 }
-
-func getSHA256(source io.ReadSeeker) (string, int64, error) {
-	hash := sha256.New()
-	size, err := io.Copy(hash, source)
-	if err != nil {
-		return "", 0, err
-	}
-	digest := hex.EncodeToString(hash.Sum(nil))
-	return digest, size, nil
-}

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-01-30 02:20:09 +0000
+++ state/apiserver/client/client.go	2014-01-30 14:23:21 +0000
@@ -4,10 +4,7 @@
 package client
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
 	"fmt"
-	"io"
 	"net/url"
 	"os"
 	"strings"
@@ -29,6 +26,7 @@
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/apiserver/common"
 	"launchpad.net/juju-core/state/statecmd"
+	"launchpad.net/juju-core/utils"
 )
 
 var logger = loggo.GetLogger("juju.state.apiserver.client")
@@ -836,9 +834,13 @@
 		return fmt.Errorf("charm URL must include revision")
 	}
 
-	// First check the charm is not already in state.
-	if _, err := c.api.state.Charm(charmURL); err == nil {
+	// First, check if a pending or a real charm exists in state.
+	stateCharm, err := c.api.state.PrepareStoreCharmUpload(charmURL)
+	if err == nil && stateCharm.IsUploaded() {
+		// Charm already in state (it was uploaded already).
 		return nil
+	} else if err != nil {
+		return err
 	}
 
 	// Get the charm and its information from the store.
@@ -862,12 +864,10 @@
 		return fmt.Errorf("cannot read downloaded charm: %v", err)
 	}
 	defer archive.Close()
-	hash := sha256.New()
-	size, err := io.Copy(hash, archive)
+	bundleSHA256, size, err := utils.ReadSHA256(archive)
 	if err != nil {
 		return fmt.Errorf("cannot calculate SHA256 hash of charm: %v", err)
 	}
-	bundleSHA256 := hex.EncodeToString(hash.Sum(nil))
 	if _, err := archive.Seek(0, 0); err != nil {
 		return fmt.Errorf("cannot rewind charm archive: %v", err)
 	}
@@ -878,12 +878,14 @@
 		return fmt.Errorf("cannot access environment: %v", err)
 	}
 	storage := env.Storage()
-	name := charm.Quote(charmURL.String())
-	err = storage.Put(name, archive, size)
+	archiveName, err := CharmArchiveName(charmURL.Name, charmURL.Revision)
 	if err != nil {
+		return fmt.Errorf("cannot generate charm archive name: %v", err)
+	}
+	if err := storage.Put(archiveName, archive, size); err != nil {
 		return fmt.Errorf("cannot upload charm to provider storage: %v", err)
 	}
-	storageURL, err := storage.URL(name)
+	storageURL, err := storage.URL(archiveName)
 	if err != nil {
 		return fmt.Errorf("cannot get storage URL for charm: %v", err)
 	}
@@ -892,7 +894,29 @@
 		return fmt.Errorf("cannot parse storage URL: %v", err)
 	}
 
-	// Finally, add the charm to state.
-	_, err = c.api.state.AddCharm(downloadedCharm, charmURL, bundleURL, bundleSHA256)
+	// Finally, update the charm data in state and mark it as no longer pending.
+	_, err = c.api.state.UpdateUploadedCharm(downloadedCharm, charmURL, bundleURL, bundleSHA256)
+	if err == state.ErrCharmRevisionAlreadyModified ||
+		state.IsCharmAlreadyUploadedError(err) {
+		// This is not an error, it just signifies somebody else
+		// managed to upload and update the charm in state before
+		// us. This means we have to delete what we just uploaded
+		// to storage.
+		if err := storage.Remove(archiveName); err != nil {
+			logger.Errorf("cannot remove duplicated charm from storage: %v", err)
+		}
+		return nil
+	}
 	return err
 }
+
+// CharmArchiveName returns a string that is suitable as a file name
+// in a storage URL. It is constructed from the charm name, revision
+// and a random UUID string.
+func CharmArchiveName(name string, revision int) (string, error) {
+	uuid, err := utils.NewUUID()
+	if err != nil {
+		return "", err
+	}
+	return charm.Quote(fmt.Sprintf("%s-%d-%s", name, revision, uuid)), nil
+}

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-01-30 03:30:30 +0000
+++ state/apiserver/client/client_test.go	2014-01-30 14:23:21 +0000
@@ -8,6 +8,8 @@
 	"net/url"
 	"strconv"
 	"strings"
+	"sync"
+	"time"
 
 	gc "launchpad.net/gocheck"
 
@@ -17,8 +19,11 @@
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/environs/storage"
+	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/provider/dummy"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
@@ -26,6 +31,7 @@
 	"launchpad.net/juju-core/state/statecmd"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/version"
 )
 
@@ -1787,6 +1793,7 @@
 	bundleURL, err := url.Parse("http://bundles.testing.invalid/" + ident)
 	c.Assert(err, gc.IsNil)
 	sch, err := s.State.AddCharm(charmDir, curl, bundleURL, ident+"-sha256")
+	c.Assert(err, gc.IsNil)
 
 	name := charm.Quote(sch.URL().String())
 	storage := s.Conn.Environ.Storage()
@@ -1804,20 +1811,127 @@
 	err = client.AddCharm(curl)
 	c.Assert(err, gc.IsNil)
 
-	// Verify it's in state.
+	// Verify it's in state and it got uploaded.
 	sch, err = s.State.Charm(curl)
 	c.Assert(err, gc.IsNil)
-
-	name = charm.Quote(curl.String())
-	storageURL, err := storage.URL(name)
-	c.Assert(err, gc.IsNil)
-
-	c.Assert(sch.BundleURL().String(), gc.Equals, storageURL)
-	// We don't care about the exact value of the hash here, just that
-	// it's set.
-	c.Assert(sch.BundleSha256(), gc.Not(gc.Equals), "")
-
-	// Verify it's added to storage.
-	_, err = storage.Get(name)
-	c.Assert(err, gc.IsNil)
+	s.assertUploaded(c, storage, sch.BundleURL(), sch.BundleSha256())
+}
+
+func (s *clientSuite) TestAddCharmConcurrently(c *gc.C) {
+	store, restore := makeMockCharmStore()
+	defer restore()
+
+	client := s.APIState.Client()
+	curl, _ := addCharm(c, store, "wordpress")
+
+	// Expect storage Put() to be called once for each goroutine
+	// below.
+	ops := make(chan dummy.Operation, 500)
+	dummy.Listen(ops)
+	go s.assertPutCalled(c, ops, 10)
+
+	// Try adding the same charm concurrently from multiple goroutines
+	// to test no "duplicate key errors" are reported (see lp bug
+	// #1067979) and also at the end only one charm document is
+	// created.
+
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func(index int) {
+			defer wg.Done()
+
+			c.Assert(client.AddCharm(curl), gc.IsNil, gc.Commentf("goroutine %d", index))
+			sch, err := s.State.Charm(curl)
+			c.Assert(err, gc.IsNil, gc.Commentf("goroutine %d", index))
+			c.Assert(sch.URL(), jc.DeepEquals, curl, gc.Commentf("goroutine %d", index))
+			expectedName := fmt.Sprintf("%s-%d-[0-9a-f-]+", curl.Name, curl.Revision)
+			c.Assert(getArchiveName(sch.BundleURL()), gc.Matches, expectedName)
+		}(i)
+	}
+	wg.Wait()
+	close(ops)
+
+	// Verify there is only a single uploaded charm remains and it
+	// contains the correct data.
+	sch, err := s.State.Charm(curl)
+	c.Assert(err, gc.IsNil)
+	storage, err := envtesting.GetEnvironStorage(s.State)
+	c.Assert(err, gc.IsNil)
+	uploads, err := storage.List(fmt.Sprintf("%s-%d-", curl.Name, curl.Revision))
+	c.Assert(err, gc.IsNil)
+	c.Assert(uploads, gc.HasLen, 1)
+	c.Assert(getArchiveName(sch.BundleURL()), gc.Equals, uploads[0])
+	s.assertUploaded(c, storage, sch.BundleURL(), sch.BundleSha256())
+}
+
+func (s *clientSuite) TestAddCharmOverwritesPlaceholders(c *gc.C) {
+	store, restore := makeMockCharmStore()
+	defer restore()
+
+	client := s.APIState.Client()
+	curl, _ := addCharm(c, store, "wordpress")
+
+	// Add a placeholder with the same charm URL.
+	err := s.State.AddStoreCharmPlaceholder(curl)
+	c.Assert(err, gc.IsNil)
+	_, err = s.State.Charm(curl)
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+
+	// Now try to add the charm, which will convert the placeholder to
+	// a pending charm.
+	err = client.AddCharm(curl)
+	c.Assert(err, gc.IsNil)
+
+	// Make sure the document's flags were reset as expected.
+	sch, err := s.State.Charm(curl)
+	c.Assert(err, gc.IsNil)
+	c.Assert(sch.URL(), jc.DeepEquals, curl)
+	c.Assert(sch.IsPlaceholder(), jc.IsFalse)
+	c.Assert(sch.IsUploaded(), jc.IsTrue)
+}
+
+func (s *clientSuite) TestCharmArchiveName(c *gc.C) {
+	for rev, name := range []string{"Foo", "bar", "wordpress", "mysql"} {
+		archiveFormat := fmt.Sprintf("%s-%d-[0-9a-f-]+", name, rev)
+		archiveName, err := client.CharmArchiveName(name, rev)
+		c.Check(err, gc.IsNil)
+		c.Check(archiveName, gc.Matches, archiveFormat)
+	}
+}
+
+func (s *clientSuite) assertPutCalled(c *gc.C, ops chan dummy.Operation, numCalls int) {
+	calls := 0
+	select {
+	case op, ok := <-ops:
+		if !ok {
+			return
+		}
+		if op, ok := op.(dummy.OpPutFile); ok {
+			calls++
+			if calls > numCalls {
+				c.Fatalf("storage Put() called %d times, expected %d times", calls, numCalls)
+				return
+			}
+			nameFormat := "[0-9a-z-]+-[0-9]+-[0-9a-f-]+"
+			c.Assert(op.FileName, gc.Matches, nameFormat)
+		}
+	case <-time.After(coretesting.LongWait):
+		c.Fatalf("timed out while waiting for a storage Put() calls")
+		return
+	}
+}
+
+func (s *clientSuite) assertUploaded(c *gc.C, storage storage.Storage, bundleURL *url.URL, expectedSHA256 string) {
+	archiveName := getArchiveName(bundleURL)
+	reader, err := storage.Get(archiveName)
+	c.Assert(err, gc.IsNil)
+	defer reader.Close()
+	downloadedSHA256, _, err := utils.ReadSHA256(reader)
+	c.Assert(err, gc.IsNil)
+	c.Assert(downloadedSHA256, gc.Equals, expectedSHA256)
+}
+
+func getArchiveName(bundleURL *url.URL) string {
+	return strings.TrimPrefix(bundleURL.RequestURI(), "/dummyenv/private/")
 }

=== modified file 'state/apiserver/export_test.go'
--- state/apiserver/export_test.go	2014-01-17 19:33:42 +0000
+++ state/apiserver/export_test.go	2014-01-30 14:23:21 +0000
@@ -6,8 +6,7 @@
 import "reflect"
 
 var (
-	RootType          = reflect.TypeOf(&srvRoot{})
-	NewPingTimeout    = newPingTimeout
-	GetEnvironStorage = getEnvironStorage
-	MaxPingInterval   = &maxPingInterval
+	RootType        = reflect.TypeOf(&srvRoot{})
+	NewPingTimeout  = newPingTimeout
+	MaxPingInterval = &maxPingInterval
 )

=== modified file 'state/state.go'
--- state/state.go	2014-01-28 04:58:43 +0000
+++ state/state.go	2014-01-30 14:23:21 +0000
@@ -445,12 +445,13 @@
 	return coll, id, nil
 }
 
-// AddCharm adds the ch charm with curl to the state.  bundleUrl must be
-// set to a URL where the bundle for ch may be downloaded from.
-// On success the newly added charm state is returned.
+// AddCharm adds the ch charm with curl to the state. bundleURL must
+// be set to a URL where the bundle for ch may be downloaded from. On
+// success the newly added charm state is returned.
 func (st *State) AddCharm(ch charm.Charm, curl *charm.URL, bundleURL *url.URL, bundleSha256 string) (stch *Charm, err error) {
-	// The charm may already exist in state as a placeholder, so we check for that situation and update the
-	// existing charm record if necessary, otherwise add a new record.
+	// The charm may already exist in state as a placeholder, so we
+	// check for that situation and update the existing charm record
+	// if necessary, otherwise add a new record.
 	var existing charmDoc
 	err = st.charms.Find(D{{"_id", curl.String()}, {"placeholder", true}}).One(&existing)
 	if err == mgo.ErrNotFound {
@@ -469,13 +470,19 @@
 	} else if err != nil {
 		return nil, err
 	}
-	return st.updateCharmDoc(ch, curl, bundleURL, bundleSha256, StillPlaceholder)
+	return st.updateCharmDoc(ch, curl, bundleURL, bundleSha256, stillPlaceholder)
 }
 
-// Charm returns the charm with the given URL.
+// Charm returns the charm with the given URL. Charms pending upload
+// to storage and placeholders are never returned.
 func (st *State) Charm(curl *charm.URL) (*Charm, error) {
 	cdoc := &charmDoc{}
-	err := st.charms.Find(D{{"_id", curl}, {"pendingupload", false}, {"placeholder", false}}).One(cdoc)
+	what := D{
+		{"_id", curl},
+		{"placeholder", D{{"$ne", true}}},
+		{"pendingupload", D{{"$ne", true}}},
+	}
+	err := st.charms.Find(what).One(&cdoc)
 	if err == mgo.ErrNotFound {
 		return nil, errors.NotFoundf("charm %q", curl)
 	}
@@ -488,7 +495,8 @@
 	return newCharm(st, cdoc)
 }
 
-// LatestPlaceholderCharm returns the latest charm described by the given URL but which is not yet deployed.
+// LatestPlaceholderCharm returns the latest charm described by the
+// given URL but which is not yet deployed.
 func (st *State) LatestPlaceholderCharm(curl *charm.URL) (*Charm, error) {
 	noRevURL := curl.WithRevision(-1)
 	curlRegex := "^" + regexp.QuoteMeta(noRevURL.String())
@@ -510,10 +518,10 @@
 	return newCharm(st, &latest)
 }
 
-// PrepareLocalCharmUpload must be called before a charm is uploaded
-// to the provider storage in order to create a charm document in
-// state. It returns a new Charm with no metadata and a unique
-// revision number.
+// PrepareLocalCharmUpload must be called before a local charm is
+// uploaded to the provider storage in order to create a charm
+// document in state. It returns the chosen unique charm URL reserved
+// in state for the charm.
 //
 // The url's schema must be "local" and it must include a revision.
 func (st *State) PrepareLocalCharmUpload(curl *charm.URL) (chosenUrl *charm.URL, err error) {
@@ -575,8 +583,95 @@
 	return chosenUrl, nil
 }
 
-var StillPending = D{{"pendingupload", true}}
-var StillPlaceholder = D{{"placeholder", true}}
+// PrepareStoreCharmUpload must be called before a charm store charm
+// is uploaded to the provider storage in order to create a charm
+// document in state. If a charm with the same URL is already in
+// state, it will be returned as a *state.Charm (is can be still
+// pending or already uploaded). Otherwise, a new charm document is
+// added in state with just the given charm URL and
+// PendingUpload=true, which is then returned as a *state.Charm.
+//
+// The url's schema must be "cs" and it must include a revision.
+func (st *State) PrepareStoreCharmUpload(curl *charm.URL) (*Charm, error) {
+	// Perform a few sanity checks first.
+	if curl.Schema != "cs" {
+		return nil, fmt.Errorf("expected charm URL with cs schema, got %q", curl)
+	}
+	if curl.Revision < 0 {
+		return nil, fmt.Errorf("expected charm URL with revision, got %q", curl)
+	}
+
+	var (
+		uploadedCharm charmDoc
+		err           error
+	)
+	for attempt := 0; attempt < 3; attempt++ {
+		// Find an uploaded or pending charm with the given exact curl.
+		err = st.charms.FindId(curl).One(&uploadedCharm)
+		if err != nil && err != mgo.ErrNotFound {
+			return nil, err
+		} else if err == nil && !uploadedCharm.Placeholder {
+			// The charm exists and it's either uploaded or still
+			// pending, but it's not a placeholder. In any case, we
+			// just return what we got.
+			return newCharm(st, &uploadedCharm)
+		} else if err == mgo.ErrNotFound {
+			// Prepare the pending charm document for insertion.
+			uploadedCharm = charmDoc{
+				URL:           curl,
+				PendingUpload: true,
+				Placeholder:   false,
+			}
+		}
+
+		var ops []txn.Op
+		if uploadedCharm.Placeholder {
+			// Convert the placeholder to a pending charm, while
+			// asserting the fields updated after an upload have not
+			// changed yet.
+			ops = []txn.Op{{
+				C:  st.charms.Name,
+				Id: curl,
+				Assert: D{
+					{"bundlesha256", ""},
+					{"pendingupload", false},
+					{"placeholder", true},
+				},
+				Update: D{{"$set", D{
+					{"pendingupload", true},
+					{"placeholder", false},
+				}}},
+			}}
+			// Update the fields of the document we're returning.
+			uploadedCharm.PendingUpload = true
+			uploadedCharm.Placeholder = false
+		} else {
+			// No charm document with this curl yet, insert it.
+			ops = []txn.Op{{
+				C:      st.charms.Name,
+				Id:     curl,
+				Assert: txn.DocMissing,
+				Insert: uploadedCharm,
+			}}
+		}
+
+		// Run the transaction, and retry on abort.
+		err = st.runTransaction(ops)
+		if err == txn.ErrAborted {
+			continue
+		} else if err != nil {
+			return nil, err
+		} else if err == nil {
+			return newCharm(st, &uploadedCharm)
+		}
+	}
+	return nil, ErrExcessiveContention
+}
+
+var (
+	stillPending     = D{{"pendingupload", true}}
+	stillPlaceholder = D{{"placeholder", true}}
+)
 
 // AddStoreCharmPlaceholder creates a charm document in state for the given charm URL which
 // must reference a charm from the store. The charm document is marked as a placeholder which
@@ -653,13 +748,39 @@
 		ops = append(ops, txn.Op{
 			C:      st.charms.Name,
 			Id:     doc.URL.String(),
-			Assert: StillPlaceholder,
+			Assert: stillPlaceholder,
 			Remove: true,
 		})
 	}
 	return ops, nil
 }
 
+// ErrCharmAlreadyUploaded is returned by UpdateUploadedCharm() when
+// the given charm is already uploaded and marked as not pending in
+// state.
+type ErrCharmAlreadyUploaded struct {
+	curl *charm.URL
+}
+
+func (e *ErrCharmAlreadyUploaded) Error() string {
+	return fmt.Sprintf("charm %q already uploaded", e.curl)
+}
+
+// IsCharmAlreadyUploadedError returns if the given error is
+// ErrCharmAlreadyUploaded.
+func IsCharmAlreadyUploadedError(err interface{}) bool {
+	if err == nil {
+		return false
+	}
+	_, ok := err.(*ErrCharmAlreadyUploaded)
+	return ok
+}
+
+// ErrCharmRevisionAlreadyModified is returned when a pending or
+// placeholder charm is no longer pending or a placeholder, signaling
+// the charm is available in state with its full information.
+var ErrCharmRevisionAlreadyModified = fmt.Errorf("charm revision already modified")
+
 // UpdateUploadedCharm marks the given charm URL as uploaded and
 // updates the rest of its data, returning it as *state.Charm.
 func (st *State) UpdateUploadedCharm(ch charm.Charm, curl *charm.URL, bundleURL *url.URL, bundleSha256 string) (*Charm, error) {
@@ -672,14 +793,16 @@
 		return nil, err
 	}
 	if !doc.PendingUpload {
-		return nil, fmt.Errorf("charm %q already uploaded", curl)
+		return nil, &ErrCharmAlreadyUploaded{curl}
 	}
 
-	return st.updateCharmDoc(ch, curl, bundleURL, bundleSha256, StillPending)
+	return st.updateCharmDoc(ch, curl, bundleURL, bundleSha256, stillPending)
 }
 
-// updateCharmDoc updates the charm with specified URL with the given data, and resets the placeholder
-// and pendingupdate flags.
+// updateCharmDoc updates the charm with specified URL with the given
+// data, and resets the placeholder and pendingupdate flags.  If the
+// charm is no longer a placeholder or pending (depending on preReq),
+// it returns ErrCharmRevisionAlreadyModified.
 func (st *State) updateCharmDoc(
 	ch charm.Charm, curl *charm.URL, bundleURL *url.URL, bundleSha256 string, preReq interface{}) (*Charm, error) {
 
@@ -698,7 +821,7 @@
 		Update: updateFields,
 	}}
 	if err := st.runTransaction(ops); err != nil {
-		return nil, onAbort(err, fmt.Errorf("charm revision already modified"))
+		return nil, onAbort(err, ErrCharmRevisionAlreadyModified)
 	}
 	return st.Charm(curl)
 }

=== modified file 'state/state_test.go'
--- state/state_test.go	2014-01-25 11:45:42 +0000
+++ state/state_test.go	2014-01-30 14:23:21 +0000
@@ -129,15 +129,26 @@
 	c.Assert(err2, jc.Satisfies, errors.IsNotFoundError)
 }
 
+func (s *StateSuite) dummyCharm(c *gc.C, curlOverride string) (ch charm.Charm, curl *charm.URL, bundleURL *url.URL, bundleSHA256 string) {
+	var err error
+	ch = testing.Charms.Dir("dummy")
+	if curlOverride != "" {
+		curl = charm.MustParseURL(curlOverride)
+	} else {
+		curl = charm.MustParseURL(
+			fmt.Sprintf("local:quantal/%s-%d", ch.Meta().Name, ch.Revision()),
+		)
+	}
+	bundleURL, err = url.Parse("http://bundles.testing.invalid/dummy-1")
+	c.Assert(err, gc.IsNil)
+	bundleSHA256 = "dummy-1-sha256"
+	return ch, curl, bundleURL, bundleSHA256
+}
+
 func (s *StateSuite) TestAddCharm(c *gc.C) {
 	// Check that adding charms from scratch works correctly.
-	ch := testing.Charms.Dir("dummy")
-	curl := charm.MustParseURL(
-		fmt.Sprintf("local:quantal/%s-%d", ch.Meta().Name, ch.Revision()),
-	)
-	bundleURL, err := url.Parse("http://bundles.testing.invalid/dummy-1")
-	c.Assert(err, gc.IsNil)
-	dummy, err := s.State.AddCharm(ch, curl, bundleURL, "dummy-1-sha256")
+	ch, curl, bundleURL, bundleSHA256 := s.dummyCharm(c, "")
+	dummy, err := s.State.AddCharm(ch, curl, bundleURL, bundleSHA256)
 	c.Assert(err, gc.IsNil)
 	c.Assert(dummy.URL().String(), gc.Equals, curl.String())
 
@@ -161,7 +172,8 @@
 	// Add a deployed charm.
 	bundleURL, err := url.Parse("http://bundles.testing.invalid/dummy-1")
 	c.Assert(err, gc.IsNil)
-	dummy, err := s.State.AddCharm(ch, curl, bundleURL, "dummy-1-sha256")
+	bundleSHA256 := "dummy-1-sha256"
+	dummy, err := s.State.AddCharm(ch, curl, bundleURL, bundleSHA256)
 	c.Assert(err, gc.IsNil)
 	c.Assert(dummy.URL().String(), gc.Equals, curl.String())
 
@@ -231,18 +243,78 @@
 	c.Assert(curl.Revision, gc.Equals, 1234)
 }
 
+func (s *StateSuite) TestPrepareStoreCharmUpload(c *gc.C) {
+	// First test the sanity checks.
+	sch, err := s.State.PrepareStoreCharmUpload(charm.MustParseURL("cs:quantal/dummy"))
+	c.Assert(err, gc.ErrorMatches, "expected charm URL with revision, got .*")
+	c.Assert(sch, gc.IsNil)
+	sch, err = s.State.PrepareStoreCharmUpload(charm.MustParseURL("local:quantal/dummy"))
+	c.Assert(err, gc.ErrorMatches, "expected charm URL with cs schema, got .*")
+	c.Assert(sch, gc.IsNil)
+
+	// No charm in state, so the call should respect given revision.
+	testCurl := charm.MustParseURL("cs:quantal/missing-123")
+	sch, err = s.State.PrepareStoreCharmUpload(testCurl)
+	c.Assert(err, gc.IsNil)
+	c.Assert(sch.URL(), gc.DeepEquals, testCurl)
+	c.Assert(sch.IsUploaded(), jc.IsFalse)
+
+	s.assertPendingCharmExists(c, sch.URL())
+
+	// Try adding it again with the same revision and ensure we get the same document.
+	schCopy, err := s.State.PrepareStoreCharmUpload(testCurl)
+	c.Assert(err, gc.IsNil)
+	c.Assert(sch, jc.DeepEquals, schCopy)
+
+	// Now add a charm and try again - we should get the same result
+	// as with AddCharm.
+	ch, curl, bundleURL, bundleSHA256 := s.dummyCharm(c, "cs:precise/dummy-2")
+	sch, err = s.State.AddCharm(ch, curl, bundleURL, bundleSHA256)
+	c.Assert(err, gc.IsNil)
+	schCopy, err = s.State.PrepareStoreCharmUpload(curl)
+	c.Assert(err, gc.IsNil)
+	c.Assert(sch, jc.DeepEquals, schCopy)
+
+	// Finally, try poking around the state with a placeholder and
+	// bundlesha256 to make sure we do the right thing.
+	curl = curl.WithRevision(999)
+	first := state.TransactionHook{
+		Before: func() {
+			err := s.State.AddStoreCharmPlaceholder(curl)
+			c.Assert(err, gc.IsNil)
+		},
+		After: func() {
+			err := s.charms.RemoveId(curl)
+			c.Assert(err, gc.IsNil)
+		},
+	}
+	second := state.TransactionHook{
+		Before: func() {
+			err := s.State.AddStoreCharmPlaceholder(curl)
+			c.Assert(err, gc.IsNil)
+		},
+		After: func() {
+			err := s.charms.UpdateId(curl, D{{"$set", D{
+				{"bundlesha256", "fake"}},
+			}})
+			c.Assert(err, gc.IsNil)
+		},
+	}
+	defer state.SetTransactionHooks(
+		c, s.State, first, second, first,
+	).Check()
+
+	_, err = s.State.PrepareStoreCharmUpload(curl)
+	c.Assert(err, gc.Equals, state.ErrExcessiveContention)
+}
+
 func (s *StateSuite) TestUpdateUploadedCharm(c *gc.C) {
-	ch := testing.Charms.Dir("dummy")
-	curl := charm.MustParseURL(
-		fmt.Sprintf("local:quantal/%s-%d", ch.Meta().Name, ch.Revision()),
-	)
-	bundleURL, err := url.Parse("http://bundles.testing.invalid/dummy-1")
-	c.Assert(err, gc.IsNil)
-	_, err = s.State.AddCharm(ch, curl, bundleURL, "dummy-1-sha256")
+	ch, curl, bundleURL, bundleSHA256 := s.dummyCharm(c, "")
+	_, err := s.State.AddCharm(ch, curl, bundleURL, bundleSHA256)
 	c.Assert(err, gc.IsNil)
 
-	// First test with already uploaded and a missing charms.
-	sch, err := s.State.UpdateUploadedCharm(ch, curl, bundleURL, "dummy-1-sha256")
+	// Test with already uploaded and a missing charms.
+	sch, err := s.State.UpdateUploadedCharm(ch, curl, bundleURL, bundleSHA256)
 	c.Assert(err, gc.ErrorMatches, fmt.Sprintf("charm %q already uploaded", curl))
 	c.Assert(sch, gc.IsNil)
 	missingCurl := charm.MustParseURL("local:quantal/missing-1")
@@ -250,7 +322,7 @@
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 	c.Assert(sch, gc.IsNil)
 
-	// Now try with an uploaded charm.
+	// Test with with an uploaded local charm.
 	_, err = s.State.PrepareLocalCharmUpload(missingCurl)
 	c.Assert(err, gc.IsNil)
 	sch, err = s.State.UpdateUploadedCharm(ch, missingCurl, bundleURL, "missing")
@@ -258,6 +330,7 @@
 	c.Assert(sch.URL(), gc.DeepEquals, missingCurl)
 	c.Assert(sch.Revision(), gc.Equals, missingCurl.Revision)
 	c.Assert(sch.IsUploaded(), jc.IsTrue)
+	c.Assert(sch.IsPlaceholder(), jc.IsFalse)
 	c.Assert(sch.Meta(), gc.DeepEquals, ch.Meta())
 	c.Assert(sch.Config(), gc.DeepEquals, ch.Config())
 	c.Assert(sch.BundleURL(), gc.DeepEquals, bundleURL)
@@ -285,11 +358,8 @@
 
 func (s *StateSuite) TestLatestPlaceholderCharm(c *gc.C) {
 	// Add a deployed charm
-	ch := testing.Charms.Dir("dummy")
-	curl := charm.MustParseURL("cs:quantal/dummy-1")
-	bundleURL, err := url.Parse("http://bundles.testing.invalid/dummy-1")
-	c.Assert(err, gc.IsNil)
-	_, err = s.State.AddCharm(ch, curl, bundleURL, "dummy-1-sha256")
+	ch, curl, bundleURL, bundleSHA256 := s.dummyCharm(c, "cs:quantal/dummy-1")
+	_, err := s.State.AddCharm(ch, curl, bundleURL, bundleSHA256)
 	c.Assert(err, gc.IsNil)
 
 	// Deployed charm not found.
@@ -341,11 +411,8 @@
 
 func (s *StateSuite) assertAddStoreCharmPlaceholder(c *gc.C) (*charm.URL, *charm.URL, *state.Charm) {
 	// Add a deployed charm
-	ch := testing.Charms.Dir("dummy")
-	curl := charm.MustParseURL("cs:quantal/dummy-1")
-	bundleURL, err := url.Parse("http://bundles.testing.invalid/dummy-1")
-	c.Assert(err, gc.IsNil)
-	dummy, err := s.State.AddCharm(ch, curl, bundleURL, "dummy-1-sha256")
+	ch, curl, bundleURL, bundleSHA256 := s.dummyCharm(c, "cs:quantal/dummy-1")
+	dummy, err := s.State.AddCharm(ch, curl, bundleURL, bundleSHA256)
 	c.Assert(err, gc.IsNil)
 
 	// Add a charm placeholder

=== modified file 'testing/charm.go'
--- testing/charm.go	2014-01-09 04:04:54 +0000
+++ testing/charm.go	2014-01-30 14:23:21 +0000
@@ -9,16 +9,11 @@
 	"os"
 	"os/exec"
 	"path/filepath"
+	"sync"
 
 	"launchpad.net/juju-core/charm"
 )
 
-func init() {
-	p, err := build.Import("launchpad.net/juju-core/testing", "", build.FindOnly)
-	check(err)
-	Charms = &Repo{Path: filepath.Join(p.Dir, "repo")}
-}
-
 func check(err error) {
 	if err != nil {
 		panic(err)
@@ -27,12 +22,27 @@
 
 // Repo represents a charm repository used for testing.
 type Repo struct {
-	Path string
+	once sync.Once
+	path string
+}
+
+func (r *Repo) Path() string {
+	r.once.Do(r.init)
+	return r.path
+}
+
+// init is called once when r.Path() is called for the first time, and
+// it initializes r.path to the location of the local testing
+// repository.
+func (r *Repo) init() {
+	p, err := build.Import("launchpad.net/juju-core/testing", "", build.FindOnly)
+	check(err)
+	r.path = filepath.Join(p.Dir, "repo")
 }
 
 // Charms represents the specific charm repository stored in this package and
 // used by the Juju unit tests. The series name is "quantal".
-var Charms *Repo
+var Charms = &Repo{}
 
 func clone(dst, src string) string {
 	check(exec.Command("cp", "-r", src, dst).Run())
@@ -42,7 +52,7 @@
 // DirPath returns the path to a charm directory with the given name in the
 // default series
 func (r *Repo) DirPath(name string) string {
-	return filepath.Join(r.Path, "quantal", name)
+	return filepath.Join(r.Path(), "quantal", name)
 }
 
 // Dir returns the actual charm.Dir named name.

=== modified file 'utils/trivial.go'
--- utils/trivial.go	2014-01-14 06:49:14 +0000
+++ utils/trivial.go	2014-01-30 14:23:21 +0000
@@ -6,8 +6,11 @@
 import (
 	"bytes"
 	"compress/gzip"
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 	"strings"
@@ -118,3 +121,26 @@
 	}
 	return ioutil.ReadAll(r)
 }
+
+// ReadSHA256 returns the SHA256 hash of the contents read from source
+// (hex encoded) and the size of the source in bytes.
+func ReadSHA256(source io.Reader) (string, int64, error) {
+	hash := sha256.New()
+	size, err := io.Copy(hash, source)
+	if err != nil {
+		return "", 0, err
+	}
+	digest := hex.EncodeToString(hash.Sum(nil))
+	return digest, size, nil
+}
+
+// ReadFileSHA256 is like ReadSHA256 but reads the contents of the
+// given file.
+func ReadFileSHA256(filename string) (string, int64, error) {
+	f, err := os.Open(filename)
+	if err != nil {
+		return "", 0, err
+	}
+	defer f.Close()
+	return ReadSHA256(f)
+}

=== modified file 'utils/trivial_test.go'
--- utils/trivial_test.go	2014-01-17 02:35:28 +0000
+++ utils/trivial_test.go	2014-01-30 14:23:21 +0000
@@ -4,6 +4,10 @@
 package utils_test
 
 import (
+	"bytes"
+	"fmt"
+	"io/ioutil"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -36,7 +40,7 @@
 	}
 )
 
-func (utilsSuite) TestCompression(c *gc.C) {
+func (*utilsSuite) TestCompression(c *gc.C) {
 	cdata := utils.Gzip(data)
 	c.Assert(len(cdata) < len(data), gc.Equals, true)
 	data1, err := utils.Gunzip(cdata)
@@ -48,7 +52,7 @@
 	c.Assert(data1, gc.DeepEquals, data)
 }
 
-func (utilsSuite) TestCommandString(c *gc.C) {
+func (*utilsSuite) TestCommandString(c *gc.C) {
 	type test struct {
 		args     []string
 		expected string
@@ -71,3 +75,43 @@
 		c.Assert(result, gc.Equals, test.expected)
 	}
 }
+
+func (*utilsSuite) TestReadSHA256AndReadFileSHA256(c *gc.C) {
+	sha256Tests := []struct {
+		content string
+		sha256  string
+	}{{
+		content: "",
+		sha256:  "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+	}, {
+		content: "some content",
+		sha256:  "290f493c44f5d63d06b374d0a5abd292fae38b92cab2fae5efefe1b0e9347f56",
+	}, {
+		content: "foo",
+		sha256:  "2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae",
+	}, {
+		content: "Foo",
+		sha256:  "1cbec737f863e4922cee63cc2ebbfaafcd1cff8b790d8cfd2e6a5d550b648afa",
+	}, {
+		content: "multi\nline\ntext\nhere",
+		sha256:  "c384f11c0294280792a44d9d6abb81f9fd991904cb7eb851a88311b04114231e",
+	}}
+
+	tempDir := c.MkDir()
+	for i, test := range sha256Tests {
+		c.Logf("test %d: %q -> %q", i, test.content, test.sha256)
+		buf := bytes.NewBufferString(test.content)
+		hash, size, err := utils.ReadSHA256(buf)
+		c.Check(err, gc.IsNil)
+		c.Check(hash, gc.Equals, test.sha256)
+		c.Check(int(size), gc.Equals, len(test.content))
+
+		tempFileName := filepath.Join(tempDir, fmt.Sprintf("sha256-%d", i))
+		err = ioutil.WriteFile(tempFileName, []byte(test.content), 0644)
+		c.Check(err, gc.IsNil)
+		fileHash, fileSize, err := utils.ReadFileSHA256(tempFileName)
+		c.Check(err, gc.IsNil)
+		c.Check(fileHash, gc.Equals, hash)
+		c.Check(fileSize, gc.Equals, size)
+	}
+}

=== modified file 'worker/uniter/charm/charm.go'
--- worker/uniter/charm/charm.go	2013-10-03 13:13:25 +0000
+++ worker/uniter/charm/charm.go	2014-01-30 14:23:21 +0000
@@ -4,10 +4,7 @@
 package charm
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
 	"fmt"
-	"io"
 	"os"
 	"path"
 
@@ -75,11 +72,10 @@
 			}
 			log.Infof("worker/uniter/charm: download complete")
 			defer st.File.Close()
-			hash := sha256.New()
-			if _, err = io.Copy(hash, st.File); err != nil {
+			actualSha256, _, err := utils.ReadSHA256(st.File)
+			if err != nil {
 				return err
 			}
-			actualSha256 := hex.EncodeToString(hash.Sum(nil))
 			archiveSha256, err := sch.ArchiveSha256()
 			if err != nil {
 				return err

=== modified file 'worker/uniter/uniter_test.go'
--- worker/uniter/uniter_test.go	2014-01-28 03:53:50 +0000
+++ worker/uniter/uniter_test.go	2014-01-30 14:23:21 +0000
@@ -5,10 +5,7 @@
 
 import (
 	"bytes"
-	"crypto/sha256"
-	"encoding/hex"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net/rpc"
 	"net/url"
@@ -1199,10 +1196,8 @@
 	err := s.dir.BundleTo(&buf)
 	c.Assert(err, gc.IsNil)
 	body := buf.Bytes()
-	hasher := sha256.New()
-	_, err = io.Copy(hasher, &buf)
+	hash, _, err := utils.ReadSHA256(&buf)
 	c.Assert(err, gc.IsNil)
-	hash := hex.EncodeToString(hasher.Sum(nil))
 	key := fmt.Sprintf("/charms/%s/%d", s.dir.Meta().Name, s.dir.Revision())
 	hurl, err := url.Parse(coretesting.Server.URL + key)
 	c.Assert(err, gc.IsNil)