[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [libvirt-glib 2/3] gconfig: Add GVirConfigDomainSnapshotDisk getters/setters



On Thu, May 02, 2013 at 11:40:56PM +0300, Zeeshan Ali (Khattak) wrote:
> On Thu, May 2, 2013 at 6:36 PM, Christophe Fergeau <cfergeau redhat com> wrote:
> > On Thu, May 02, 2013 at 06:22:14PM +0300, Zeeshan Ali (Khattak) wrote:
> >> On Wed, May 1, 2013 at 10:59 PM, Christophe Fergeau <cfergeau redhat com> wrote:
> >> > +
> >> > +
> >> > +const char *gvir_config_domain_snapshot_disk_get_driver_type(GVirConfigDomainSnapshotDisk *disk)
> >>
> >> Shouldn't 'driver_type' be an enum?
> >
> > libvirt is storing it internally as a string, see
> > http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/conf/snapshot_conf.c;h=5b54a28a596aef0bb883dfe12ca5c2f300a48999;hb=HEAD#l130
> > libvirt-gconfig generally follows what libvirt does when it comes to
> > choosing between strings and enums.
> 
> I don't think that is true cause I see examples of enum setter/getters
> that translate to strings in libvirt xml. e.g GVirConfigDomainOsType,

This one is indeed inconsistent, maybe it was added before danpb gave this
rule of thumb for enum VS string, maybe the inconsistency was not noticed
at review time.

> GVirConfigDomainOsSmBiosMode

VIR_ENUM_IMPL(virDomainSmbiosMode, VIR_DOMAIN_SMBIOS_LAST,
              "none",
              "emulate",
              "host",
              "sysinfo")

> and GVirConfigDomainOsBootDevice.

VIR_ENUM_IMPL(virDomainBoot, VIR_DOMAIN_BOOT_LAST,
              "fd",
              "cdrom",
              "hd",
              "network")

> If we go with string option, I suggest we provide #defines for known
> values and document here that these can be used.

Sounds good, though this could be done in an additional patch as this would
also be useful for disk devices.

However, after a bit more research,
http://libvirt.org/git/?p=libvirt.git;a=commit;h=e2c41e486018ee74f6a75c1f717622
makes the enum case more compelling.

Christophe

Attachment: pgp7AjFPOyfBf.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]