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(a)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(a)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...
> 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=e2c41e486018ee74f6a75c1f...
makes the enum case more compelling.
Christophe
Attachments:
- attachment.sig (application/pgp-signature — 198 bytes)