[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]