On Tue, Mar 15, 2011 at 11:07:53AM -0600, Eric Blake wrote:
On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
* tools/virsh.c: Add vol-create-upload, vol-upload and vol-download commands --- .x-sc_avoid_write | 1 + tools/virsh.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 0 deletions(-)
diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write index f6fc1b2..0784984 100644 --- a/.x-sc_avoid_write +++ b/.x-sc_avoid_write @@ -5,5 +5,6 @@ ^src/util/util\.c$ ^src/xen/xend_internal\.c$ ^daemon/libvirtd.c$ +^tools/virsh\.c$
Jim Meyering has a pending upstream patch for gnulib that will get rid of the .x-sc* files in favor of cfg.mk; if that goes in first, it will affect this patch.
Do we really need to add an unprotected write() to virsh.c? That's such a big file to exempt, and it would be nicer if we could just exempt a helper file that does the single usage while keeping the overall virsh.c file protected.
Yes, I can in fact change it to saferead/safewrite. This was a leftover from earlier code
+/* + * "vol-upload" command + */ +static const vshCmdInfo info_vol_upload[] = { + {"help", gettext_noop("upload a file into a volume")},
I thought we had a syntax-check rule that required you to use N_ instead of gettext_noop here. If we don't, that's probably worth adding, to enforce consistency with the rest of the file.
Opps, this was a really old code I forgot.
+ {"desc", gettext_noop("Upload a file into a volume")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_upload[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")},
The above are required,
+ {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") }, + {"length", VSH_OT_INT, 0, N_("amount of data to upload") },
But these two should be optional (offset of 0, length of entire file [and that in turn depends on answering my question in patch 2/5 about whether 0 for length behaves special, or whether we need some other sentinel like -1]).
Err, they are optional already - '0' instead of VSH_OFLAG_REQ.
+ unsigned long long offset, length; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + offset = 0; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + length = 0;
Not quite the right usage pattern. If the result is < 0, then that was a syntax error (such as --offset=foo) and should be diagnosed. Meanwhile, if the result is 0, then this uses offset uninitialized.
Yep, I see this now. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|