12:07 p.m.
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.
^gnulib/
^tools/console.c$
diff --git a/tools/virsh.c b/tools/virsh.c
index 42ebd55..9c4eac8 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -263,6 +263,9 @@ static int vshCommandOptString(const vshCmd *cmd, const char *name,
static int vshCommandOptLongLong(const vshCmd *cmd, const char *name,
long long *value)
ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
+static int vshCommandOptULongLong(const vshCmd *cmd, const char *name,
+ unsigned long long *value)
+ ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
Is it worth adding the helper API in a separate patch, to make it easier
to cherry pick just vshCommandOptULongLong and some followup patch that
uses it independently of vol-upload?
+/*
+ * "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.
+ {"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]).
+ 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.
+ if (virStreamSendAll(st, cmdVolUploadSource, &fd) < 0) {
+ vshError(ctl, "cannot send data to volume %s", name);
+ goto cleanup;
+ }
+
+ if (close(fd) < 0) {
s/close/VIR_CLOSE/
+ vshError(ctl, "cannot close file %s", file);
+ virStreamAbort(st);
+ goto cleanup;
+ }
+ fd = -1;
then you don't need this line.
+ if (fd != -1)
+ close(fd);
VIR_FORCE_CLOSE - 'make syntax-check' should have caught this
+ return ret;
+}
+
+
+
+/*
+ * "vol-download" command
+ */
+static const vshCmdInfo info_vol_download[] = {
+ {"help", gettext_noop("Download a volume to a file")},
+ {"desc", gettext_noop("Download a volume to a file")},
N_()
+ {NULL, NULL}
+};
+
+static const vshCmdOptDef opts_vol_download[] = {
+ {"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")},
+ {"offset", VSH_OT_INT, 0, N_("volume offset to download from")
},
+ {"length", VSH_OT_INT, 0, N_("amount of data to download") },
+ {NULL, 0, 0, NULL}
+};
+
+
+static int
+cmdVolDownloadSink(virStreamPtr st ATTRIBUTE_UNUSED,
+ const char *bytes, size_t nbytes, void *opaque)
+{
+ int *fd = opaque;
+
+ return write(*fd, bytes, nbytes);
+}
Consistency - for upload, you put the helper before the
vshCmdInfo/vshCmdOptDef declarations, but for download, you put it in
between the declarations and their associated function. I like the
style used for upload.
+
+ if (vshCommandOptULongLong(cmd, "offset", &offset) < 0)
+ offset = 0;
Same usage problems as for upload.
+
+ if (vshCommandOptULongLong(cmd, "length", &length) < 0)
+ length = 0;
+
+ if (vshCommandOptString(cmd, "file", &file) < 0)
+ goto cleanup;
+
+ if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) {
Should --mode be allowed as an optional argument?
+
+ if (close(fd) < 0) {
VIR_CLOSE
+
/*
* "net-edit" command
*/
@@ -10534,6 +10736,7 @@ static const vshCmdDef storageVolCmds[] = {
{"vol-create", cmdVolCreate, opts_vol_create, info_vol_create},
{"vol-create-from", cmdVolCreateFrom, opts_vol_create_from,
info_vol_create_from},
{"vol-delete", cmdVolDelete, opts_vol_delete, info_vol_delete},
+ {"vol-download", cmdVolDownload, opts_vol_download, info_vol_download },
{"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml},
{"vol-info", cmdVolInfo, opts_vol_info, info_vol_info},
{"vol-key", cmdVolKey, opts_vol_key, info_vol_key},
@@ -10541,6 +10744,7 @@ static const vshCmdDef storageVolCmds[] = {
{"vol-name", cmdVolName, opts_vol_name, info_vol_name},
{"vol-path", cmdVolPath, opts_vol_path, info_vol_path},
{"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool},
+ {"vol-upload", cmdVolUpload, opts_vol_upload, info_vol_upload },
{"vol-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe},
{NULL, NULL, NULL, NULL}
No vol-create-upload command? Is the commit message out-of-date, or is
this patch incomplete?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org