[libvirt] [PATCH] Sheepdog: Auto Adding volume and correct refresh volume problem.
From: Joel SIMOES <joel.simoes@laposte.net> --- src/storage/storage_backend_sheepdog.c | 91 ++++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index a6981ce..fbed11a 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -86,7 +86,8 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool, pool->available = pool->capacity - pool->allocation; return 0; - } while ((p = next)); + } + while ((p = next)); return -1; } @@ -109,6 +110,71 @@ virStorageBackendSheepdogAddHostArg(virCommandPtr cmd, virCommandAddArgFormat(cmd, "%d", port); } +static int +virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool) +{ + int ret; + char *output = NULL; + + virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", "-r", NULL); + virStorageBackendSheepdogAddHostArg(cmd, pool); + virCommandSetOutputBuffer(cmd, &output); + ret = virCommandRun(cmd, NULL); + char** lines; + char** cells; + + if (ret < 0) + goto cleanup; + + lines = virStringSplit(output, "\n", 0); + size_t i; + for (i = 0; *(lines + i); i++) { + char *line = *(lines + i); + cells = virStringSplit(line, " ", 0); + size_t j; + for (j = 0; *(cells + j); j++) { + + char *cell = *(cells + j); + if (j == 1) { + virStorageVolDefPtr vol = NULL; + if (VIR_ALLOC(vol) < 0) + goto cleanup; + + if (VIR_STRDUP(vol->name, cell) < 0) + goto cleanup; + + vol->type = VIR_STORAGE_VOL_BLOCK; + + if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0) + goto cleanup; + pool->volumes.objs[pool->volumes.count - 1] = vol; + + if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0) + goto cleanup; + + vol = NULL; + } + + VIR_FREE(*(cells + j)); + } + + VIR_FREE(cells); + VIR_FREE(*(lines + i)); + } + + + VIR_FREE(lines); + + + + +cleanup: + virCommandFree(cmd); + VIR_FREE(lines); + VIR_FREE(cells); + return ret; +} static int virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -122,15 +188,16 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); ret = virCommandRun(cmd, NULL); - if (ret == 0) - ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output); + if (ret < 0) + goto cleanup; + ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output); + virStorageBackendSheepdogRefreshAllVol(conn, pool); +cleanup: virCommandFree(cmd); - VIR_FREE(output); return ret; } - static int virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, @@ -143,12 +210,14 @@ virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "delete", vol->name, NULL); virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); + if (ret < 0) + goto cleanup; +cleanup: virCommandFree(cmd); return ret; } - static int virStorageBackendSheepdogCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, @@ -174,7 +243,6 @@ virStorageBackendSheepdogCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; } - static int virStorageBackendSheepdogBuildVol(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -195,12 +263,12 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn, goto cleanup; ret = 0; + cleanup: virCommandFree(cmd); return ret; } - int virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol, char *output) @@ -257,7 +325,8 @@ virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol, return -1; return 0; - } while ((p = next)); + } + while ((p = next)); return -1; } @@ -295,7 +364,6 @@ cleanup: return ret; } - static int virStorageBackendSheepdogResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, @@ -310,7 +378,10 @@ virStorageBackendSheepdogResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCommandAddArgFormat(cmd, "%llu", capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); + if (ret < 0) + goto cleanup; +cleanup: virCommandFree(cmd); return ret; -- 1.8.3.2
On 01/22/2014 08:19 AM, joel SIMOES wrote:
From: Joel SIMOES <joel.simoes@laposte.net>
The commit message body is a great place to give more details about what the actual "refresh volume problem" is that you encountered
--- src/storage/storage_backend_sheepdog.c | 91 ++++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index a6981ce..fbed11a 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -86,7 +86,8 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool, pool->available = pool->capacity - pool->allocation; return 0;
- } while ((p = next)); + } + while ((p = next));
This hunk is not needed.
+static int +virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool) +{ + int ret; + char *output = NULL; + + virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", "-r", NULL); + virStorageBackendSheepdogAddHostArg(cmd, pool); + virCommandSetOutputBuffer(cmd, &output); + ret = virCommandRun(cmd, NULL); + char** lines; + char** cells;
Style wise, I'd hoist these two declarations to the top with the other declarations (in this particular case, your code is syntactically valid and won't trigger any compiler warnings, but if a later person inserts a 'goto cleanup' prior to these lines, the compiler might start warning about skipped variable declarations, which is why our HACKING documents declarations up front in C89 style even though we require a C99 compiler).
+ + if (ret < 0) + goto cleanup; + + lines = virStringSplit(output, "\n", 0); + size_t i;
And _this_ declaration is indeed after a conditional goto, also worth hoisting.
+ for (i = 0; *(lines + i); i++) {
*(lines + i) is more idiomatically written as lines[i]
+ char *line = *(lines + i); + cells = virStringSplit(line, " ", 0); + size_t j; + for (j = 0; *(cells + j); j++) {
cells[j]
+ + char *cell = *(cells + j); + if (j == 1) { + virStorageVolDefPtr vol = NULL; + if (VIR_ALLOC(vol) < 0) + goto cleanup; + + if (VIR_STRDUP(vol->name, cell) < 0) + goto cleanup;
Memory leak. vol is allocated but goes out of scope by way of the goto.
+ + vol->type = VIR_STORAGE_VOL_BLOCK; + + if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0) + goto cleanup;
More memory leaks.
+ pool->volumes.objs[pool->volumes.count - 1] = vol; + + if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0) + goto cleanup; + + vol = NULL; + } + + VIR_FREE(*(cells + j));
Again, VIR_FREE(cells[j]) looks nicer. ...
+ ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output); + virStorageBackendSheepdogRefreshAllVol(conn, pool);
Why are you ignoring the return status of this call?
@@ -143,12 +210,14 @@ virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "delete", vol->name, NULL); virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); + if (ret < 0) + goto cleanup;
+cleanup:
This hunk does nothing, and isn't needed.
virCommandFree(cmd); return ret; }
- static int
Spurious whitespace change (while we don't enforce it, most of our code uses two blank lines between functions).
@@ -195,12 +263,12 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn, goto cleanup;
ret = 0; + cleanup: virCommandFree(cmd); return ret; }
- int
More spurious whitespace changes.
virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol, char *output) @@ -257,7 +325,8 @@ virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol, return -1;
return 0; - } while ((p = next)); + } + while ((p = next));
Another spurious hunk.
@@ -310,7 +378,10 @@ virStorageBackendSheepdogResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCommandAddArgFormat(cmd, "%llu", capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); int ret = virCommandRun(cmd, NULL); + if (ret < 0) + goto cleanup;
+cleanup:
Another no-op change. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/22/2014 04:19 PM, joel SIMOES wrote:
From: Joel SIMOES <joel.simoes@laposte.net>
--- src/storage/storage_backend_sheepdog.c | 91 ++++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index a6981ce..fbed11a 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -109,6 +110,71 @@ virStorageBackendSheepdogAddHostArg(virCommandPtr cmd, virCommandAddArgFormat(cmd, "%d", port); }
+static int +virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool) +{ + int ret; + char *output = NULL; + + virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", "-r", NULL); + virStorageBackendSheepdogAddHostArg(cmd, pool); + virCommandSetOutputBuffer(cmd, &output); + ret = virCommandRun(cmd, NULL);
If the command is run succesfully, ret gets set to 0 and the function returns 0 even if something fails. You don't use the return value of virCommandRun later, so you can just do: if (virCommandRun(cmd, NULL) < 0) goto cleanup; And set ret = 0 just before the cleanup: label, as we do in most of libvirt.
+ char** lines; + char** cells; + + if (ret < 0) + goto cleanup; + + lines = virStringSplit(output, "\n", 0); + size_t i; + for (i = 0; *(lines + i); i++) { + char *line = *(lines + i); + cells = virStringSplit(line, " ", 0); + size_t j; + for (j = 0; *(cells + j); j++) { + + char *cell = *(cells + j); + if (j == 1) { + virStorageVolDefPtr vol = NULL; + if (VIR_ALLOC(vol) < 0) + goto cleanup; + + if (VIR_STRDUP(vol->name, cell) < 0) + goto cleanup; + + vol->type = VIR_STORAGE_VOL_BLOCK; + + if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0) + goto cleanup; + pool->volumes.objs[pool->volumes.count - 1] = vol; + + if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0) + goto cleanup; + + vol = NULL; + } + + VIR_FREE(*(cells + j));
We also have a virStringFreeList function, that frees the list generated by virStringSplit. You wouldn't need the inner 'for' loop then.
+ } + + VIR_FREE(cells); + VIR_FREE(*(lines + i)); + } + + + VIR_FREE(lines); + + + + +cleanup: + virCommandFree(cmd); + VIR_FREE(lines); + VIR_FREE(cells); + return ret; +}
static int virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
Jan
participants (3)
-
Eric Blake -
joel SIMOES -
Ján Tomko