On 06/08/2010 06:03 AM, Justin Clift wrote:

Hi all,

Much better - git am liked this patch. And a tip for your matching your commit message style to other commits: list a category first, use all lower-case, and end without a period. While not essential, doing these things will make 'git log --oneline' produce more readable output.

This patch adds a "vol-pool" command to virsh, to round out the conversion functions for vols in virsh.

At the moment, if we start with a volume name and a pool id, we're all good. We can convert from that pair to either a volume key or volume path.

Also, wrap commit messages, like you would email (72 or 80 columns is typical).

But, if we're given just a volume key or path we have no good way to look up the storage pool for it. Kind of makes it a one way association. :/

This patch fixes that, making it possible to work from just having a volume key or path. :)

Regards and best wishes,

Justin Clift

--- tools/virsh.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 48 insertions(+), 0 deletions(-)

No change to tools/virsh.pod? I know you have other cleanup patches to virsh.pod, but for a new feature, I'd rather get in the habit of checking in the doc change at the same time as the new feature.

diff --git a/tools/virsh.c b/tools/virsh.c index 1279f41..95aea0f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5975,6 +5975,53 @@ cmdVolName(vshControl *ctl, const vshCmd *cmd) }

+/* + * "vol-pool" command + */ +static const vshCmdInfo info_vol_pool[] = { + {"help", N_("returns the storage pool for a given volume key or path")}, + {"desc", ""}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_pool[] = { + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("volume key or path")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdVolPool(vshControl *ctl, const vshCmd *cmd) +{ + virStoragePoolPtr pool; + virStorageVolPtr vol; + + // Check the connection to libvirtd daemon is still working

The rest of this file used /* */ comments. It's nice to be consistent (even though we require C99 for various other reasons, we're still stuck on some C89 syntax constructs for style reasons).

+ if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + // Use the supplied string to locate the volume + if (!(vol = vshCommandOptVolBy(ctl, cmd, "vol", "pool", NULL, + VSH_BYUUID))) { + return FALSE; + } + + // Look up the parent storage pool for the volume + pool = virStoragePoolLookupByVolume(vol); + if (pool == NULL) { + vshError(ctl, "%s", _("failed to get parent pool")); + virStorageVolFree(vol); + return FALSE; + } + + // Return the name of the parent storage pool + vshPrint(ctl, "%s\n", virStoragePoolGetName(pool)); + + // Cleanup + virStorageVolFree(vol); + virStoragePoolFree(pool); + return TRUE; +} +

/* * "vol-key" command @@ -8837,6 +8884,7 @@ static const vshCmdDef commands[] = { {"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml}, {"vol-info", cmdVolInfo, opts_vol_info, info_vol_info}, {"vol-list", cmdVolList, opts_vol_list, info_vol_list}, + {"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool}, {"vol-path", cmdVolPath, opts_vol_path, info_vol_path}, {"vol-name", cmdVolName, opts_vol_name, info_vol_name}, {"vol-key", cmdVolKey, opts_vol_key, info_vol_key},

In general, this looks okay to me, but I'll wait for the next round addressing the nits I raised before pushing anything. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org