[libvirt] [PATCH 3/5] Add vol-create-upload, vol-upload and vol-download commands to virsh
Eric Blake
eblake at redhat.com
Tue Mar 15 17:07:53 UTC 2011
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110315/76e07128/attachment-0001.sig>
More information about the libvir-list
mailing list