Re: [libvirt] [PATCH 3/5] Add vol-create-upload, vol-upload and vol-download commands to virsh

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.

> +/* > + * "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.