Re: [Patch]Fix bugs of Sheepdog storage driver

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

 



On 2013年02月08日 13:42, harryxiyou wrote:
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.

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


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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]