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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org