[libvirt] [PATCH 2/3] virsh: Use VIR_ENUM* macros for vshCmdAttachInterface

[Daniel P. Berrange]

On Mon, Feb 09, 2015 at 04:02:04PM +0100, Ján Tomko wrote:
> On Mon, Feb 09, 2015 at 09:47:47AM -0500, Laine Stump wrote:
> > On 02/09/2015 08:19 AM, Ján Tomko wrote:
> > > On Mon, Feb 09, 2015 at 10:58:29AM +0100, Michal Privoznik wrote:
> > >> Instead of verbose string to enum conversion (if STREQ() else if
> > >> STREQ() else if STREQ() ...) lets use awesome VIR_ENUM_* macros.
> > >>
> > >> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > >> ---
> > >>  tools/virsh-domain.c | 28 ++++++++++++++++++----------
> > >>  1 file changed, 18 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > >> index b465eb5..c7e4926 100644
> > >> --- a/tools/virsh-domain.c
> > >> +++ b/tools/virsh-domain.c
> > >> @@ -855,10 +855,16 @@ static int parseRateStr(const char *rateStr, virNetDevBandwidthRatePtr rate)
> > >>  }
> > >>  
> > >>  typedef enum {
> > >> -    IFACE_TYPE_NETWORK,
> > >> +    IFACE_TYPE_NETWORK = 0,
> > >>      IFACE_TYPE_BRIDGE,
> > >> +    IFACE_TYPE_LAST,
> > >>  } vshCmdAttachInterfaceType;
> > >>  
> > >> +VIR_ENUM_DECL(vshCmdAttachInterface)
> > >> +VIR_ENUM_IMPL(vshCmdAttachInterface, IFACE_TYPE_LAST,
> > >> +              "network",
> > >> +              "bridge")
> > >> +
> > >>  static bool
> > >>  cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> > >>  {
> > >> @@ -906,11 +912,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> > >>          goto cleanup;
> > >>  
> > >>      /* check interface type */
> > >> -    if (STREQ(type, "network")) {
> > >> -        typ = IFACE_TYPE_NETWORK;
> > >> -    } else if (STREQ(type, "bridge")) {
> > >> -        typ = IFACE_TYPE_BRIDGE;
> > >> -    } else {
> > >> +    if ((typ = vshCmdAttachInterfaceTypeFromString(type)) < 0) {
> > >>          vshError(ctl, _("No support for %s in command 'attach-interface'"),
> > >>                   type);
> > >>          goto cleanup;
> > >> @@ -943,10 +945,16 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
> > >>      virBufferAsprintf(&buf, "<interface type='%s'>\n", type);
> > > Here, the type argument is already assumed to use values from the virDomainNetType enum.
> > > I think we can reuse that type here, instead of creating a new enum.
> > 
> > Except that virDomainNetType is not a part of libvirt's public API, and
> > so is not available to an application program like virsh.
> > 
> 
> We already include conf/*.h files in virsh.
> 
> Should they be removed to make virsh a cleaner example of a client app?
> Or left in to avoid code duplication?

I think virsh should aim to only use the public API in general. If we
find the public API is insufficient for virsh, it is probably also
insufficient for other apps, and as such we should be looking at
extending the public API rather than letting virsh access private
methods.

Regards,
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 :|