[libvirt] [PATCH] docs: fix documentation of enum constants
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly. Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */ + VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT = 2, + VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT = 3, /* * Page faults occur when a process makes a valid access to virtual memory * that is not available. When servicing the page fault, if disk IO is * required, it is considered a major fault. If not, it is a minor fault. * These are expressed as the number of faults that have occurred. */ - VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT = 2, - VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT = 3, + VIR_DOMAIN_MEMORY_STAT_UNUSED = 4, /* * The amount of memory left completely unused by the system. Memory that * is available but used for reclaimable caches should NOT be reported as * free. This value is expressed in kB. */ - VIR_DOMAIN_MEMORY_STAT_UNUSED = 4, + VIR_DOMAIN_MEMORY_STAT_AVAILABLE = 5, /* * The total amount of usable memory as seen by the domain. This value * may be less than the amount of memory assigned to the domain if a * balloon driver is in use or if the guest OS does not initialize all * assigned pages. This value is expressed in kB. */ - VIR_DOMAIN_MEMORY_STAT_AVAILABLE = 5, - /* Current balloon value (in KB). */ VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON = 6, + /* Current balloon value (in KB). */ + VIR_DOMAIN_MEMORY_STAT_RSS = 7, /* Resident Set Size of the process running the domain. This value * is in kB */ - VIR_DOMAIN_MEMORY_STAT_RSS = 7, + VIR_DOMAIN_MEMORY_STAT_USABLE = 8, /* * How much the balloon can be inflated without pushing the guest system * to swap, corresponds to 'Available' in /proc/meminfo */ - VIR_DOMAIN_MEMORY_STAT_USABLE = 8, - /* Timestamp of the last update of statistics, in seconds. */ VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE = 9, + /* Timestamp of the last update of statistics, in seconds. */ + VIR_DOMAIN_MEMORY_STAT_NR = 10, /* * The number of statistics supported by this version of the interface. * To add new statistics, add them to the enum and increase this value. */ - VIR_DOMAIN_MEMORY_STAT_NR = 10, # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_MEMORY_STAT_LAST = VIR_DOMAIN_MEMORY_STAT_NR @@ -683,22 +683,23 @@ typedef enum { /* Domain migration flags. */ typedef enum { + VIR_MIGRATE_LIVE = (1 << 0), /* Do not pause the domain during migration. The domain's memory will * be transferred to the destination host while the domain is running. * The migration may never converge if the domain is changing its memory * faster then it can be transferred. The domain can be manually paused * anytime during migration using virDomainSuspend. */ - VIR_MIGRATE_LIVE = (1 << 0), + VIR_MIGRATE_PEER2PEER = (1 << 1), /* Tell the source libvirtd to connect directly to the destination host. * Without this flag the client (e.g., virsh) connects to both hosts and * controls the migration process. In peer-to-peer mode, the source * libvirtd controls the migration by calling the destination daemon * directly. */ - VIR_MIGRATE_PEER2PEER = (1 << 1), + VIR_MIGRATE_TUNNELLED = (1 << 2), /* Tunnel migration data over libvirtd connection. Without this flag the * source hypervisor sends migration data directly to the destination * hypervisor. This flag can only be used when VIR_MIGRATE_PEER2PEER is @@ -707,26 +708,26 @@ typedef enum { * Note the less-common spelling that we're stuck with: * VIR_MIGRATE_TUNNELLED should be VIR_MIGRATE_TUNNELED. */ - VIR_MIGRATE_TUNNELLED = (1 << 2), + VIR_MIGRATE_PERSIST_DEST = (1 << 3), /* Define the domain as persistent on the destination host after successful * migration. If the domain was persistent on the source host and * VIR_MIGRATE_UNDEFINE_SOURCE is not used, it will end up persistent on * both hosts. */ - VIR_MIGRATE_PERSIST_DEST = (1 << 3), + VIR_MIGRATE_UNDEFINE_SOURCE = (1 << 4), /* Undefine the domain on the source host once migration successfully * finishes. */ - VIR_MIGRATE_UNDEFINE_SOURCE = (1 << 4), + VIR_MIGRATE_PAUSED = (1 << 5), /* Leave the domain suspended on the destination host. virDomainResume (on * the virDomainPtr returned by the migration API) has to be called * explicitly to resume domain's virtual CPUs. */ - VIR_MIGRATE_PAUSED = (1 << 5), + VIR_MIGRATE_NON_SHARED_DISK = (1 << 6), /* Migrate full disk images in addition to domain's memory. By default * only non-shared non-readonly disk images are transferred. The * VIR_MIGRATE_PARAM_MIGRATE_DISKS parameter can be used to specify which @@ -734,8 +735,8 @@ typedef enum { * * This flag and VIR_MIGRATE_NON_SHARED_INC are mutually exclusive. */ - VIR_MIGRATE_NON_SHARED_DISK = (1 << 6), + VIR_MIGRATE_NON_SHARED_INC = (1 << 7), /* Migrate disk images in addition to domain's memory. This is similar to * VIR_MIGRATE_NON_SHARED_DISK, but only the top level of each disk's * backing chain is copied. That is, the rest of the backing chain is @@ -744,15 +745,15 @@ typedef enum { * * This flag and VIR_MIGRATE_NON_SHARED_DISK are mutually exclusive. */ - VIR_MIGRATE_NON_SHARED_INC = (1 << 7), + VIR_MIGRATE_CHANGE_PROTECTION = (1 << 8), /* Protect against domain configuration changes during the migration * process. This flag is used automatically when both sides support it. * Explicitly setting this flag will cause migration to fail if either the * source or the destination does not support it. */ - VIR_MIGRATE_CHANGE_PROTECTION = (1 << 8), + VIR_MIGRATE_UNSAFE = (1 << 9), /* Force migration even if it is considered unsafe. In some cases libvirt * may refuse to migrate the domain because doing so may lead to potential * problems such as data corruption, and thus the migration is considered @@ -761,8 +762,8 @@ typedef enum { * is unsafe unless the disk images are stored on coherent clustered * filesystem, such as GFS2 or GPFS. */ - VIR_MIGRATE_UNSAFE = (1 << 9), + VIR_MIGRATE_OFFLINE = (1 << 10), /* Migrate a domain definition without starting the domain on the * destination and without stopping it on the source host. Offline * migration requires VIR_MIGRATE_PERSIST_DEST to be set. @@ -770,28 +771,28 @@ typedef enum { * Offline migration may not copy disk storage or any other file based * storage (such as UEFI variables). */ - VIR_MIGRATE_OFFLINE = (1 << 10), + VIR_MIGRATE_COMPRESSED = (1 << 11), /* Compress migration data. The compression methods can be specified using * VIR_MIGRATE_PARAM_COMPRESSION. A hypervisor default method will be used * if this parameter is omitted. Individual compression methods can be * tuned via their specific VIR_MIGRATE_PARAM_COMPRESSION_* parameters. */ - VIR_MIGRATE_COMPRESSED = (1 << 11), + VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* Cancel migration if a soft error (such as I/O error) happens during * migration. */ - VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), + VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* Enable algorithms that ensure a live migration will eventually converge. * This usually means the domain will be slowed down to make sure it does * not change its memory faster than a hypervisor can transfer the changed * memory to the destination host. VIR_MIGRATE_PARAM_AUTO_CONVERGE_* * parameters can be used to tune the algorithm. */ - VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), + VIR_MIGRATE_RDMA_PIN_ALL = (1 << 14), /* This flag can be used with RDMA migration (i.e., when * VIR_MIGRATE_PARAM_URI starts with "rdma://") to tell the hypervisor * to pin all domain's memory at once before migration starts rather then @@ -807,21 +808,20 @@ typedef enum { * domain and the host itself since the host's kernel may run out of * memory. */ - VIR_MIGRATE_RDMA_PIN_ALL = (1 << 14), + VIR_MIGRATE_POSTCOPY = (1 << 15), /* Setting the VIR_MIGRATE_POSTCOPY flag tells libvirt to enable post-copy * migration. However, the migration will start normally and * virDomainMigrateStartPostCopy needs to be called to switch it into the * post-copy mode. See virDomainMigrateStartPostCopy for more details. */ - VIR_MIGRATE_POSTCOPY = (1 << 15), + VIR_MIGRATE_TLS = (1 << 16), /* Setting the VIR_MIGRATE_TLS flag will cause the migration to attempt * to use the TLS environment configured by the hypervisor in order to * perform the migration. If incorrectly configured on either source or * destination, the migration will fail. */ - VIR_MIGRATE_TLS = (1 << 16), } virDomainMigrateFlags; @@ -2986,16 +2986,16 @@ typedef enum { * Details on the cause of a 'shutdown' lifecycle event */ typedef enum { - /* Guest finished shutdown sequence */ VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0, + /* Guest finished shutdown sequence */ + VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1, /* Domain finished shutting down after request from the guest itself * (e.g. hardware-specific action) */ - VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1, + VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2, /* Domain finished shutting down after request from the host (e.g. killed by * a signal) */ - VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2, # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_SHUTDOWN_LAST -- 2.13.1
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator. I recall this being discussed somewhere recently (probably on the list?). The proper fix IMO is to fix the generator so that it accepts both: enum { /* Some very long description - therefore it's before the value. */ VIR_ENUM_A_VAL = 0, } virEnumA; enum { VIR_ENUM_B_VAL = 0, /* Short description */ } virEnumB; Michal
On Fri, 21 Jul 2017 10:12:38 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator.
I agree that it's not ideal solution. But since it's already used in the header, e.g. in virDomainBlockJobType and virDomainEventShutdownDetailType, I assumed it's acceptable. And the overall readability is not that bad because the constant+doc pairs are separated with newline from one another.
I recall this being discussed somewhere recently (probably on the list?). The proper fix IMO is to fix the generator so that it accepts both:
That would be the best solution, but I'm too scared to look at the generator. That would be a job for somebody else. Tomas
enum { /* Some very long description - therefore it's before the value. */ VIR_ENUM_A_VAL = 0, } virEnumA;
enum { VIR_ENUM_B_VAL = 0, /* Short description */ } virEnumB;
Michal
-- Tomáš Golembiovský <tgolembi@redhat.com>
On Fri, Jul 21, 2017 at 12:25:02PM +0200, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 10:12:38 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator.
I agree that it's not ideal solution. But since it's already used in the header, e.g. in virDomainBlockJobType and virDomainEventShutdownDetailType, I assumed it's acceptable. And the overall readability is not that bad because the constant+doc pairs are separated with newline from one another.
I'm afraid, those examples are bad too and should never have been committed as they are, so not a good guide to follow. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 10:12:38 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator.
I agree that it's not ideal solution. But since it's already used in the header, e.g. in virDomainBlockJobType and virDomainEventShutdownDetailType,
This one actually looks okay. Did you perhaps mean virConnectDomainEventDiskChangeReason?
I assumed it's acceptable. And the overall readability is not that bad because the constant+doc pairs are separated with newline from one another.
Nope. It's not. I've sent a patch that fixes those two places (I've went through all of our public headers and found just those two though): https://www.redhat.com/archives/libvir-list/2017-July/msg00871.html
I recall this being discussed somewhere recently (probably on the list?). The proper fix IMO is to fix the generator so that it accepts both:
That would be the best solution, but I'm too scared to look at the generator. That would be a job for somebody else.
Yeah. Our generator is not that great. I wish that we'd switch to something already out there so that we don't have to maintain our own generator. Michal
On Fri, Jul 21, 2017 at 12:58:55PM +0200, Michal Privoznik wrote:
On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 10:12:38 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator.
I agree that it's not ideal solution. But since it's already used in the header, e.g. in virDomainBlockJobType and virDomainEventShutdownDetailType,
This one actually looks okay. Did you perhaps mean virConnectDomainEventDiskChangeReason?
I assumed it's acceptable. And the overall readability is not that bad because the constant+doc pairs are separated with newline from one another.
Nope. It's not. I've sent a patch that fixes those two places (I've went through all of our public headers and found just those two though):
https://www.redhat.com/archives/libvir-list/2017-July/msg00871.html
I recall this being discussed somewhere recently (probably on the list?). The proper fix IMO is to fix the generator so that it accepts both:
That would be the best solution, but I'm too scared to look at the generator. That would be a job for somebody else.
Yeah. Our generator is not that great. I wish that we'd switch to something already out there so that we don't have to maintain our own generator.
There's a good few options for docs generators. Unfortunately our code is also used for generating the XML API description, that's in turn used to generate some language bindings, and I don't know of any tools that could replace that part. So it looks like we're stuck as long as we want to support auto-generated bindings from the XML description Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 07/21/2017 01:01 PM, Daniel P. Berrange wrote:
On Fri, Jul 21, 2017 at 12:58:55PM +0200, Michal Privoznik wrote:
<snip/> Yeah. Our generator is not that great. I wish that we'd switch to something already out there so that we don't have to maintain our own generator.
There's a good few options for docs generators. Unfortunately our code is also used for generating the XML API description, that's in turn used to generate some language bindings, and I don't know of any tools that could replace that part. So it looks like we're stuck as long as we want to support auto-generated bindings from the XML description
Well we can use proper doc generator for generating doc and then our own generator for generating the XML API description. BTW: what good options do you have in mind? Michal
On Fri, Jul 21, 2017 at 01:03:13PM +0200, Michal Privoznik wrote:
On 07/21/2017 01:01 PM, Daniel P. Berrange wrote:
On Fri, Jul 21, 2017 at 12:58:55PM +0200, Michal Privoznik wrote:
<snip/> Yeah. Our generator is not that great. I wish that we'd switch to something already out there so that we don't have to maintain our own generator.
There's a good few options for docs generators. Unfortunately our code is also used for generating the XML API description, that's in turn used to generate some language bindings, and I don't know of any tools that could replace that part. So it looks like we're stuck as long as we want to support auto-generated bindings from the XML description
Well we can use proper doc generator for generating doc and then our own generator for generating the XML API description. BTW: what good options do you have in mind?
Doxygen, Sphinx, GTK-DOC, though I would not be in favour of Doxygen as IMHO the docs it generates have awful navigation. QEMU is switching to use Sphinx IIRC. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, 21 Jul 2017 12:58:55 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 10:12:38 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator.
I agree that it's not ideal solution. But since it's already used in the header, e.g. in virDomainBlockJobType and virDomainEventShutdownDetailType,
This one actually looks okay. Did you perhaps mean virConnectDomainEventDiskChangeReason?
No, I really meant virDomainEventShutdownDetailType. But you're right about virConnectDomainEventDiskChangeReason too. I didn't look at other headers, but as far as libvirt-domain.h is concerned those three seem to be all. There are also several (5?) misplaced comments for the sentinels if you care about those too.
I assumed it's acceptable. And the overall readability is not that bad because the constant+doc pairs are separated with newline from one another.
Nope. It's not. I've sent a patch that fixes those two places (I've went through all of our public headers and found just those two though):
https://www.redhat.com/archives/libvir-list/2017-July/msg00871.html
I recall this being discussed somewhere recently (probably on the list?). The proper fix IMO is to fix the generator so that it accepts both:
That would be the best solution, but I'm too scared to look at the generator. That would be a job for somebody else.
Yeah. Our generator is not that great. I wish that we'd switch to something already out there so that we don't have to maintain our own generator.
Michal
-- Tomáš Golembiovský <tgolembi@redhat.com>
On 07/21/2017 01:17 PM, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 12:58:55 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 10:12:38 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator.
I agree that it's not ideal solution. But since it's already used in the header, e.g. in virDomainBlockJobType and virDomainEventShutdownDetailType,
This one actually looks okay. Did you perhaps mean virConnectDomainEventDiskChangeReason?
No, I really meant virDomainEventShutdownDetailType.
Are we looking at the same code? Here's what mine looks like: typedef enum { /* Guest finished shutdown sequence */ VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0, /* Domain finished shutting down after request from the guest itself * (e.g. hardware-specific action) */ VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1, /* Domain finished shutting down after request from the host (e.g. killed by * a signal) */ VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2, # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_SHUTDOWN_LAST # endif } virDomainEventShutdownDetailType; I see nothing wrong about it. Do you? Michal
On Fri, 21 Jul 2017 13:26:14 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/21/2017 01:17 PM, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 12:58:55 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
On Fri, 21 Jul 2017 10:12:38 +0200 Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused. The problem is in our doc generator.
I agree that it's not ideal solution. But since it's already used in the header, e.g. in virDomainBlockJobType and virDomainEventShutdownDetailType,
This one actually looks okay. Did you perhaps mean virConnectDomainEventDiskChangeReason?
No, I really meant virDomainEventShutdownDetailType.
Are we looking at the same code? Here's what mine looks like:
Damn! We're not. I've been looking at my patched version and virDomainEventShutdownDetailType was changed by me. Sorry for confusion. Tomas
typedef enum { /* Guest finished shutdown sequence */ VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0,
/* Domain finished shutting down after request from the guest itself * (e.g. hardware-specific action) */ VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1,
/* Domain finished shutting down after request from the host (e.g. killed by * a signal) */ VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2,
# ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_SHUTDOWN_LAST # endif } virDomainEventShutdownDetailType;
I see nothing wrong about it. Do you?
Michal
-- Tomáš Golembiovský <tgolembi@redhat.com>
On Fri, Jul 21, 2017 at 10:12:38AM +0200, Michal Privoznik wrote:
On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:
The documentation string has to follow the definition of a constant in the enum. Otherwise, the HTML documentation will be generated incorrectly.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 45f939a8c..2f3162d0f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr; * Memory Statistics Tags: */ typedef enum { - /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0, - /* The total amount of memory written out to swap space (in kB). */ + /* The total amount of data read from swap space (in kB). */ VIR_DOMAIN_MEMORY_STAT_SWAP_OUT = 1, + /* The total amount of memory written out to swap space (in kB). */
While this fixes generated HTML, it messes up the header file. So if somebody is looking directly into header file they might get confused.
Yeah, absolutely NACK to this approach. Putting comments after the constant name is awful for people reading the code.
The problem is in our doc generator. I recall this being discussed somewhere recently (probably on the list?). The proper fix IMO is to fix the generator so that it accepts both:
enum { /* Some very long description - therefore it's before the value. */ VIR_ENUM_A_VAL = 0, } virEnumA;
enum { VIR_ENUM_B_VAL = 0, /* Short description */ } virEnumB;
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Tomáš Golembiovský