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

On Fri, Feb 8, 2013 at 1:06 PM, Osier Yang<jyang(a)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(a)googlegroups.com" ML to the CC'ed list.

> 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

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

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?