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