On 06/06/10 - 07:09:37PM, Justin Clift wrote:

--- tools/virsh.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 7 ++++++- 2 files changed, 57 insertions(+), 2 deletions(-)

Hey Justin, There are a few issues with this patch stylistically: 1) First, it didn't apply here; it seems like carriage returns were added at 80 characters, which made for a corrupt patch. I know you mentioned you were going to use git-send-mail for patches; did you use it for this one? 2) While comments are definitely appreciated, we use the C-style /* */ for comments instead of C++ style //. 3) Please wrap lines at 80 columns wherever possible. You don't have to do it in the middle of strings, but splitting along commas and other punctuation makes the patches easier to read. A few more comments inline...

diff --git a/tools/virsh.c b/tools/virsh.c index 1279f41..3ba549b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5274,6 +5274,8 @@ static const vshCmdOptDef opts_vol_create_as[] = { {"capacity", VSH_OT_DATA, VSH_OFLAG_REQ, N_("size of the vol with optional k,M,G,T suffix")}, {"allocation", VSH_OT_STRING, 0, N_("initial allocation size with optional k,M,G,T suffix")}, {"format", VSH_OT_STRING, 0, N_("file format type raw,bochs,qcow,qcow2,vmdk")}, + {"snapshot-source-vol", VSH_OT_STRING, 0, N_("the source volume if taking a snapshot")}, + {"snapshot-source-format", VSH_OT_STRING, 0, N_("format of the source volume if taking a snapshot")}, {NULL, 0, 0, NULL} }; @@ -5313,6 +5315,7 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) int found; char *xml; char *name, *capacityStr, *allocationStr, *format; + char *snapshotStrVol, *snapshotStrFormat; unsigned long long capacity, allocation = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5339,6 +5342,8 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("Malformed size %s"), allocationStr); format = vshCommandOptString(cmd, "format", &found); + snapshotStrVol = vshCommandOptString(cmd, "snapshot-source-vol", &found); + snapshotStrFormat = vshCommandOptString(cmd, "snapshot-source-format", &found); virBufferAddLit(&buf, "<volume>\n"); virBufferVSprintf(&buf, " <name>%s</name>\n", name); @@ -5351,8 +5356,53 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) virBufferVSprintf(&buf, " <format type='%s'/>\n",format); virBufferAddLit(&buf, " </target>\n"); } - virBufferAddLit(&buf, "</volume>\n"); + // Convert the snapshot parameters into backingStore XML + if (snapshotStrVol) { + // Use the name of the snapshot source volume to retrieve its device path + vshDebug(ctl, 5, "%s: Looking up backing store volume '%s' as name\n", cmd->def->name, snapshotStrVol); + virStorageVolPtr snapVol = virStorageVolLookupByName(pool, snapshotStrVol);

We don't typically mix declarations and code. It's actually not entirely true (you'll see examples of it if you look around hard enough), but it's kind of surprising to see new variables in the middle of blocks of code. Please move this to the top.

+ if (snapVol) + vshDebug(ctl, 5, "%s: Backing store volume found using '%s' as name\n", cmd->def->name, snapshotStrVol); + + if (snapVol == NULL) { + // Snapshot source volume was not found by name. Try using the snapshot-source-vol parameter as a key + vshDebug(ctl, 5, "%s: Looking up backing store volume '%s' as key\n", cmd->def->name, snapshotStrVol); + snapVol = virStorageVolLookupByKey(ctl->conn, snapshotStrVol); + if (snapVol) + vshDebug(ctl, 5, "%s: Backing store volume found using '%s' as key\n", cmd->def->name, snapshotStrVol); + } + if (snapVol == NULL) { + // Snapshot source volume was not found by key either. Try using the snapshot-source-vol parameter as a path + vshDebug(ctl, 5, "%s: Looking up backing store volume '%s' by path\n", cmd->def->name, snapshotStrVol); + snapVol = virStorageVolLookupByPath(ctl->conn, snapshotStrVol); + if (snapVol) + vshDebug(ctl, 5, "%s: Backing store volume found using '%s' as path\n", cmd->def->name, snapshotStrVol); + } + if (snapVol == NULL) { + vshError(ctl, _("failed to get vol '%s'"), snapshotStrVol); + return FALSE; + } + + char *snapshotStrVolPath; + if ((snapshotStrVolPath = virStorageVolGetPath(snapVol)) == NULL) { + virStorageVolFree(snapVol); + return FALSE; + } + + // Create the XML for the backing store + virBufferAddLit(&buf, " <backingStore>\n"); + virBufferVSprintf(&buf, " <path>%s</path>\n",snapshotStrVolPath);

I'm not entirely certain because of the broken patch, but it seems like you need to add 4 spaces before <path>.

+ if (snapshotStrFormat) + virBufferVSprintf(&buf, " <format type='%s'/>\n",snapshotStrFormat);

Here, you'll have to handle the case where the user passed in NULL for snapshotStrFormat.

+ virBufferAddLit(&buf, " </backingStore>\n"); + + // Cleanup snapshot allocations + VIR_FREE(snapshotStrVolPath); + virStorageVolFree(snapVol); + } + + virBufferAddLit(&buf, "</volume>\n");

Overall, it looks OK. I'm not a huge fan of the separation of --snapshot-source-vol and --snapshot-source-format, since it makes it too easy to mess up one or the other. On the other hand, it is a bit easier than making XML by hand, and I don't have any better ideas, so maybe we should leave it as-is. If we do leave it, I think you'll want to add some code to check for all 4 cases of: snapshotStrVol == NULL, snapshotStrFormat == NULL snapshotStrVol == specified, snapshotStrFormat == NULL snapshotStrVol == specified, snapshotStrFormat == specified snapshotStrVol == NULL, snapshotStrFormat == specified -- Chris Lalancette