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

[Osier Yang]

On 2013年02月08日 13:57, Osier Yang wrote:
> 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;

Btw, if you look further more on virStorageBackendSheepdogRefreshVol,
you will see it has bugs too. It can be another patch though.

>
> 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?
>>
>>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list