[sheepdog] [libvirt] [Patch]Fix bugs of Sheepdog storage driver

[Osier Yang]

On 2013年02月08日 13:42, harryxiyou wrote:
> On Fri, Feb 8, 2013 at 1:06 PM, Osier Yang<jyang at redhat.com>  wrote:
> [...]
>> Google Groups小组敬上
>>
>> That's bad if one could get one notice like this each time after
>> reviewing your patch. :-) Not sure how to get rid of this, but
>> I think it's caused by the permission, so please either change
>> the google group to public, or don't cc to the list when posting
>> patch.
>>
>
> Thanks, it's because i add "cloudxy at googlegroups.com" ML to the
> CC'ed list.

Yes, see:

 >> I think it's caused by the permission, so please either change
 >> the google group to public, or don't cc to the list when posting
 >> patch.

>
>>> You sent same patch multiple times. It might be caused by the mail
>>> delay though, I.E, you don't see the one sent out earlier. Perhaps
>>> you should check you mailbox setting to avoid the small flood.
>>>
> Sorry, i am not familiar with 'git send-email --compose' well. So it sends
> several times. I will fix this problem.
>
> [...]
>>> So, consider to change the commit log to:
>>>
>>> Don't try to refresh the volume if creating volume fails.
>
> This one is well for me, thanks.
>
>>>>
>>>> Signed-off-by: Harry Wei<harryxiyou at gmail.com>
>>>> ---
>>>> src/storage/storage_backend_sheepdog.c | 5 ++++-
>>>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/src/storage/storage_backend_sheepdog.c
>>>> b/src/storage/storage_backend_sheepdog.c
>>>> index cd18f33..f987604 100644
>>>> --- a/src/storage/storage_backend_sheepdog.c
>>>> +++ b/src/storage/storage_backend_sheepdog.c
>>>> @@ -168,9 +168,12 @@ virStorageBackendSheepdogCreateVol(virConnectPtr
>>>> conn ATTRIBUTE_UNUSED,
>>>> virCommandAddArgFormat(cmd, "%llu", vol->capacity);
>>>> virStorageBackendSheepdogAddHostArg(cmd, pool);
>>>> ret = virCommandRun(cmd, NULL);
>>>> + if (ret<  0)
>>>> + goto cleanup;
>>>
>>>
>>> We prefer:
>>>
>>> if ((ret = virCommandRun(cmd, NULL))<  0)
>>> goto cleanup;
>>>
>>> However, you can avoid using the "ret" here by initialize it
>>> to "-1", with that you can simply do:
>>>
>>> if (virCommandRun(cmd, NULL)<  0)
>>> goto cleanup;
>>>
>>>>
>>>> - virStorageBackendSheepdogRefreshVol(conn, pool, vol);
>>>> + ret = virStorageBackendSheepdogRefreshVol(conn, pool, vol);
>>>
>>>
>>> And:
>>>
>>> if (virStorageBackendSheepdogRefreshVol(conn, pool, vol)<  0)
>>> goto cleanup;
>>>
>>> ret = 0;
>>>
>>>>
>>>> +cleanup:
>>>> virCommandFree(cmd);
>>>> return ret;
>>>> }
>
> We cannot do like
>

No true.

> [...]
>   if (virCommandRun(cmd, NULL)<  0)
>      goto cleanup;
> [...]
>   if (virStorageBackendSheepdogRefreshVol(conn, pool, vol)<  0)
>      goto cleanup;
> [...]
> +cleanup:
>         virCommandFree(cmd);
>         return ret;
>
> We must initialize "ret",

That's why I said in previous replyment "initialize ret", I mean
this (see the top of the function):

     int ret = -1;

And with setting "ret" to 0 before cleanup:

     ret = 0;
cleanup:
     virCommandFree(cmd);
     return ret;

because if either virCommandRun and
> virStorageBackendSheepdogRefreshVol has some errors, they
> will return '-1' to "ret". Then execute "return ret", which who calls
> virStorageBackendSheepdogCreateVol will know it has happened
> some errors. So we should modify them like this.
>
> [...]
>
>      if ((ret = virCommandRun(cmd, NULL))<  0)
>             goto cleanup;
> [...]
>     if (ret = virStorageBackendSheepdogRefreshVol(conn, pool, vol))<  0)
>             goto cleanup;
>
> cleanup:
>       virCommandFree(cmd);
>       return ret;
>   }
>
>   Osier Yang, do you agree with me?
>
>