Re: [Patch]Fix bugs of Sheepdog storage driver

[harryxiyou <harryxiyou@xxxxxxxxx>]

On Fri, Feb 8, 2013 at 1:06 PM, Osier Yang <jyang@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx" ML to the
CC'ed list.

>> 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@xxxxxxxxx>
>>> ---
>>> 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

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

We must initialize "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?


-- 
Thanks
Harry Wei

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list