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

[Daniel P. Berrange]

On Tue, Mar 15, 2011 at 11:07:53AM -0600, Eric Blake wrote:
> 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.

Yes, I can in fact change it to saferead/safewrite. This was a
leftover from earlier code

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

Opps, this was a really old code I forgot.

> > +    {"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]).

Err, they are optional already - '0' instead of VSH_OFLAG_REQ.

> 
> > +    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.

Yep, I see this now.



Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|