Re: [libvirt] [PATCH 3/4] virsh: New command cmdChangeMedia

[Osier Yang <jyang redhat com>]

On 02/14/2012 06:18 AM, Eric Blake wrote:
> On 02/03/2012 02:23 AM, Osier Yang wrote:
> > One could use it to eject, insert, or update media of the CDROM
> > or floppy drive. See the documents for more details.
> > ---
> >   tools/virsh.c |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 files changed, 134 insertions(+), 0 deletions(-)
>
> I would squash 3/4 and 4/4 into one patch (new commands should be
> committed with documentation).
>
> > +
> > +    if ((eject&&  insert) ||
> > +        (insert&&  update) ||
> > +        (eject&&  update)) {
> > +        vshError(ctl, "%s", _("--eject, --insert, and --update must be specified "
> > +                            "exclusively."));
> > +        return false;
> > +    }
> > +
> > +    if (!eject&&  !insert&&  !update) {
> > +        vshError(ctl, "%s", _("One of --eject, --insert, and --update should be "
> > +                              "specified"));
> > +        return false;
> > +    }
>
> Seems reasonable.
>
> > +
> > +    if (eject)
> > +        prepare_flags |= VSH_PREPARE_DISK_XML_EJECT;
> > +
> > +    if (insert)
> > +        prepare_flags |= VSH_PREPARE_DISK_XML_INSERT;
> > +
> > +    if (update)
> > +        prepare_flags |= VSH_PREPARE_DISK_XML_UPDATE;
>
> But that means that you are using these as an enum, not as a bitmask to
> be OR'd together.

Yeah, it should be enum.

> > +
> > +    if (insert&&  !source) {
> > +        vshError(ctl, "%s", _("No disk source specified for inserting"));
> > +        goto cleanup;
> > +    }
>
> Shouldn't you also error out if (eject&&  source)?

"source" is just ignored if (eject).

> > +    if (virDomainUpdateDeviceFlags(dom, disk_xml, flags) != 0) {
> > +        vshError(ctl, "%s", _("Failed to insert media"));
>
> The error message is a bit misleading in the --eject case, since we
> didn't insert media, but removed it.

Ah, yes, will update.

> But overall, I think this is a long over-due command.