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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list