Re: [libvirt] [PATCH 5/5] list: Use virConnectListAllSecrets in virsh

On 09/14/12 10:38, Osier Yang wrote: > This introduce four new options for secret-list, to filter the s/introduce/introduces/ > returned secrets by whether it's ephemeral or not, and/or by > whether it's private or not. > > * tools/virsh-secret.c: (New helper vshSecretSorter, > vshSecretListFree, and vshCollectSecretList; Use the new > API for secret-list; error out if flags are specified, > because there is no way to filter the results when using > old APIs (no APIs to get the properties (ephemeral, private) > of a secret yet). > > * tools/virsh.pod: Document the 4 new options. > --- > tools/virsh-secret.c | 209 > +++++++++++++++++++++++++++++++++++++++++++------- > tools/virsh.pod | 8 ++- > 2 files changed, 186 insertions(+), 31 deletions(-) > > diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c > index abcfdfe..ab50405 100644 > --- a/tools/virsh-secret.c > +++ b/tools/virsh-secret.c > @@ -290,6 +290,141 @@ cleanup: > return ret; > } > > +static int > +vshSecretSorter(const void *a, const void *b) > +{ > + virSecretPtr *sa = (virSecretPtr *) a; > + virSecretPtr *sb = (virSecretPtr *) b; > + char uuid_sa[VIR_UUID_STRING_BUFLEN]; > + char uuid_sb[VIR_UUID_STRING_BUFLEN]; > + > + if (*sa && !*sb) > + return -1; > + > + if (!*sa) > + return *sb != NULL; > + > + virSecretGetUUIDString(*sa, uuid_sa); > + virSecretGetUUIDString(*sb, uuid_sb); > + > + return vshStrcasecmp(uuid_sa, uuid_sb); > +} > + > +struct vshSecretList { > + virSecretPtr *secrets; > + size_t nsecrets; > +}; > +typedef struct vshSecretList *vshSecretListPtr; > + > +static void > +vshSecretListFree(vshSecretListPtr list) > +{ > + int i; > + > + if (list && list->nsecrets) { > + for (i = 0; i < list->nsecrets; i++) { > + if (7list->secrets[i]) > + virSecretFree(list->secrets[i]); > + } > + VIR_FREE(list->secrets); > + } > + VIR_FREE(list); > +} > + > +static vshSecretListPtr > +vshSecretListCollect(vshControl *ctl, > + unsigned int flags) > +{ > + vshSecretListPtr list = vshMalloc(ctl, sizeof(*list)); > + int i; > + int ret; > + virSecretPtr secret; > + bool success = false; > + size_t deleted = 0; > + int nsecrets = 0; > + char **uuids = NULL; > + > + /* try the list with flags support (0.10.2 and later) */ > + if ((ret = virConnectListAllSecrets(ctl->conn, > + &list->secrets, > + flags)) >= 0) { > + list->nsecrets = ret; > + goto finished; > + } > + > + /* check if the command is actually supported */ > + if (last_error && last_error->code == VIR_ERR_NO_SUPPORT) > + goto fallback; > + > + /* there was an error during the call */ > + vshError(ctl, "%s", _("Failed to list node secrets")); > + goto cleanup; > + > + > +fallback: > + /* fall back to old method (0.10.1 and older) */ > + vshResetLibvirtError(); > + > + if (flags) { > + vshError(ctl, "%s", _("Filtering is not supported by this libvirt")); > + goto cleanup; > + } > + > + nsecrets = virConnectNumOfSecrets(ctl->conn); > + if (nsecrets < 0) { > + vshError(ctl, "%s", _("Failed to count secrets")); > + goto cleanup; > + } > + > + if (nsecrets == 0) > + return list; > + > + uuids = vshMalloc(ctl, sizeof(char *) * nsecrets); > + > + nsecrets = virConnectListSecrets(ctl->conn, uuids, nsecrets); > + if (nsecrets < 0) { > + vshError(ctl, "%s", _("Failed to list secrets")); > + goto cleanup; > + } > + > + list->secrets = vshMalloc(ctl, sizeof(virSecretPtr) * (nsecrets)); > + list->nsecrets = 0; > + > + /* get the secrets */ > + for (i = 0; i < nsecrets ; i++) { > + if (!(secret = virSecretLookupByUUIDString(ctl->conn, uuids[i]))) > + continue; > + list->secrets[list->nsecrets++] = secret; > + } > + > + /* truncate secrets that weren't found */ > + deleted = nsecrets - list->nsecrets; > + > +finished: > + /* sort the list */ > + if (list->secrets && list->nsecrets) > + qsort(list->secrets, list->nsecrets, > + sizeof(*list->secrets), vshSecretSorter); > + > + /* truncate the list for not found secret objects */ > + if (deleted) > + VIR_SHRINK_N(list->secrets, list->nsecrets, deleted); > + > + success = true; > + > +cleanup: > + for (i = 0; i < nsecrets; i++) > + VIR_FREE(uuids[i]); > + VIR_FREE(uuids); > + > + if (!success) { > + vshSecretListFree(list); > + list = NULL; > + } > + > + return list; > +} > + > /* > * "secret-list" command > */ > @@ -299,59 +434,75 @@ static const vshCmdInfo info_secret_list[] = { > {NULL, NULL} > }; > > +static const vshCmdOptDef opts_secret_list[] = { > + {"ephemeral", VSH_OT_BOOL, 0, N_("list ephemeral secrets")}, > + {"no-ephemeral", VSH_OT_BOOL, 0, N_("list secrets not ephemeral")}, I'd change the help string to: "list non-ephmeral secrets" > + {"private", VSH_OT_BOOL, 0, N_("list private secrets")}, > + {"no-private", VSH_OT_BOOL, 0, N_("list secrets not private")}, same here. > + {NULL, 0, 0, NULL} > +}; > + > static bool > cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) > { > - int maxuuids = 0, i; > - char **uuids = NULL; > + int i; > + vshSecretListPtr list = NULL; > + bool ret = false; > + bool ephemeral = vshCommandOptBool(cmd, "ephemeral"); > + bool no_ephemeral = vshCommandOptBool(cmd, "no-ephemeral"); > + bool private = vshCommandOptBool(cmd, "private"); > + bool no_private = vshCommandOptBool(cmd, "no-private"); > + unsigned int flags = 0; > There's no need to store the command options in separate bool variables as they're used only once to create the flag argument. Use them directly in the conditions that set the flags. > - maxuuids = virConnectNumOfSecrets(ctl->conn); > - if (maxuuids < 0) { > - vshError(ctl, "%s", _("Failed to list secrets")); > - return false; > - } > - uuids = vshMalloc(ctl, sizeof(*uuids) * maxuuids); > + if (ephemeral) > + flags |= VIR_CONNECT_LIST_SECRETS_EPHEMERAL; > > - maxuuids = virConnectListSecrets(ctl->conn, uuids, maxuuids); > - if (maxuuids < 0) { > - vshError(ctl, "%s", _("Failed to list secrets")); > - VIR_FREE(uuids); > - return false; > - } > + if (no_ephemeral) > + flags |= VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL; > + > + if (private) > + flags |= VIR_CONNECT_LIST_SECRETS_PRIVATE; > > - qsort(uuids, maxuuids, sizeof(char *), vshNameSorter); > + if (no_private) > + flags |= VIR_CONNECT_LIST_SECRETS_NO_PRIVATE; > + > + if (!(list = vshSecretListCollect(ctl, flags))) > + return false; > > vshPrintExtra(ctl, "%-36s %s\n", _("UUID"), _("Usage")); > vshPrintExtra(ctl, > "-----------------------------------------------------------\n"); > > - for (i = 0; i < maxuuids; i++) { > - virSecretPtr sec = virSecretLookupByUUIDString(ctl->conn, uuids[i]); > + for (i = 0; i < list->nsecrets; i++) { > + virSecretPtr sec = list->secrets[i]; > const char *usageType = NULL; > > - if (!sec) { > - VIR_FREE(uuids[i]); > - continue; > - } > - > switch (virSecretGetUsageType(sec)) { > case VIR_SECRET_USAGE_TYPE_VOLUME: > usageType = _("Volume"); > break; > } > > + char uuid[VIR_UUID_STRING_BUFLEN]; > + if (virSecretGetUUIDString(list->secrets[i], uuid) < 0) { > + vshError(ctl, "%s", _("Failed to get uuid of secret")); > + goto cleanup; > + } > + > if (usageType) { > vshPrint(ctl, "%-36s %s %s\n", > - uuids[i], usageType, > + uuid, usageType, > virSecretGetUsageID(sec)); > } else { > vshPrint(ctl, "%-36s %s\n", > - uuids[i], _("Unused")); > + uuid, _("Unused")); > } > - virSecretFree(sec); > - VIR_FREE(uuids[i]); > } > - VIR_FREE(uuids); > - return true; > + > + ret = true; > + > +cleanup: > + vshSecretListFree(list); > + return ret; > } > > const vshCmdDef secretCmds[] = { > @@ -361,7 +512,7 @@ const vshCmdDef secretCmds[] = { > info_secret_dumpxml, 0}, > {"secret-get-value", cmdSecretGetValue, opts_secret_get_value, > info_secret_get_value, 0}, > - {"secret-list", cmdSecretList, NULL, info_secret_list, 0}, > + {"secret-list", cmdSecretList, opts_secret_list, info_secret_list, 0}, > {"secret-set-value", cmdSecretSetValue, opts_secret_set_value, > info_secret_set_value, 0}, > {"secret-undefine", cmdSecretUndefine, opts_secret_undefine, > diff --git a/tools/virsh.pod b/tools/virsh.pod > index a6390c2..9d35502 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -2469,9 +2469,13 @@ encoded using Base64. > Delete a I<secret> (specified by its UUID), including the associated > value, if > any. > > -=item B<secret-list> > +=item B<secret-list> [I<--ephemeral>] [I<--no-ephemeral>] > + [I<--private>] [I<--no-private>] > > -Output a list of UUIDs of known secrets to stdout. > +Returns the list of secrets. You may also want to filter the returned > secrets > +by I<--ephemeral> to list the ephemeral ones, I<--no-ephemeral> to > list the > +not ephemeral ones, I<--private> to list the private ones, and non-ephmeral > +I<--no-private> to list the ones not private. not private ones.