Re: [libvirt] [Patch]Fix bugs of Sheepdog storage driver

On 2013年02月08日 13:42, harryxiyou wrote: > On Fri, Feb 8, 2013 at 1:06 PM, Osier Yang&lt;jyang(a)redhat.com&gt; 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 &quot;cloudxy(a)googlegroups.com&quot; 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&lt;harryxiyou(a)gmail.com&gt; >>>> --- >>>> 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? > > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list