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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list