[libvirt] [PATCH 1/4] virDomainIOError public API and remote protocol

[Michal Privoznik]

On 25.01.2012 12:04, Michal Privoznik wrote:
> On 23.01.2012 14:30, Jiri Denemark wrote:
>> We already provide ways to detect when a domain has been paused as a
>> result of I/O error, but there was no way of getting the exact error or
>> even the device that experienced it.  This new API may be used for both.
>> ---
>>  include/libvirt/libvirt.h.in |   19 +++++++++++++++
>>  src/driver.h                 |    6 ++++
>>  src/libvirt.c                |   53 ++++++++++++++++++++++++++++++++++++++++++
>>  src/libvirt_public.syms      |    5 ++++
>>  src/remote/remote_driver.c   |    1 +
>>  src/remote/remote_protocol.x |   13 +++++++++-
>>  src/remote_protocol-structs  |    9 +++++++
>>  7 files changed, 105 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index 958e5a6..2c3423a 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -3746,6 +3746,25 @@ int virConnectSetKeepAlive(virConnectPtr conn,
>>                             int interval,
>>                             unsigned int count);
>>  
>> +/**
>> + * virDomainIoError:
>> + *
>> + * Disk I/O error.
>> + */
>> +typedef enum {
>> +    VIR_DOMAIN_IOERROR_NONE     = 0, /* no error */
>> +    VIR_DOMAIN_IOERROR_NO_SPACE = 1, /* no space left on the device */
>> +    VIR_DOMAIN_IOERROR_UNSPEC   = 2, /* unspecified I/O error */
> 
> Just cosmetic, since this is expected to be extended one day, I'd like
> to see _UNSPEC either at the beginning or at the end, but not in the
> middle. However, the latter is not possible as it's API change.
> But I can live with this thought.
> 
>> +
>> +#ifdef VIR_ENUM_SENTINELS
>> +    VIR_DOMAIN_IOERROR_LAST
>> +#endif
>> +} virDomainIoError;
>> +
>> +int virDomainGetIoError(virDomainPtr dom,
>> +                        const char *dev,
>> +                        unsigned int flags);
>> +
>>  #ifdef __cplusplus
>>  }
>>  #endif
>> diff --git a/src/driver.h b/src/driver.h
>> index 24636a4..1dd3760 100644
>> --- a/src/driver.h
>> +++ b/src/driver.h
>> @@ -794,6 +794,11 @@ typedef int
>>                                    int *nparams,
>>                                    unsigned int flags);
>>  
>> +typedef int
>> +    (*virDrvDomainGetIoError)(virDomainPtr dom,
>> +                              const char *dev,
>> +                              unsigned int flags);
>> +
>>  /**
>>   * _virDriver:
>>   *
>> @@ -962,6 +967,7 @@ struct _virDriver {
>>      virDrvNodeSuspendForDuration nodeSuspendForDuration;
>>      virDrvDomainSetBlockIoTune domainSetBlockIoTune;
>>      virDrvDomainGetBlockIoTune domainGetBlockIoTune;
>> +    virDrvDomainGetIoError domainGetIoError;
>>  };
>>  
>>  typedef int
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index 7b8adf7..99edca3 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -17882,3 +17882,56 @@ error:
>>      virDispatchError(dom->conn);
>>      return -1;
>>  }
>> +
>> +/**
>> + * virDomainGetIoError:
>> + * @dom: a domain object
>> + * @dev: disk device to be inspected or NULL
>> + * @flags: extra flags; not used yet, so callers should always pass 0
>> + *
>> + * The @disk parameter is either the device target (the dev attribute of
>> + * target subelement), such as "vda", or NULL, in which case all disks will be
>> + * inspected for errors. If only one disk experienced an I/O error, that error
>> + * will be returned. However, if there are more disks with I/O errors, this
>> + * function will fail and return -2, requiring the caller to check every
>> + * device explicitly.
>> + *
>> + * Returns -2 if @dev is NULL and there are multiple disks with errors, -1 if
>> + * the function fails to do its job, the I/O error (virDomainIoError) observed
>> + * on the specified disk (or any disk if @dev is NULL). Namely, 0 is returned
>> + * when there is no error on the disk.
>> + */
>> +int
>> +virDomainGetIoError(virDomainPtr dom,
>> +                    const char *dev,
>> +                    unsigned int flags)
> 
> Do we want to narrow this only for disks? Maybe some day hypervisors
> will report IO errors on other devices as well (haven't tried to find
> out if they do now days, though). However, if we restrict this only for
> disks, I'd change the name, so it is obvious that it's disk-only, e.g.
> virDomainGetDiskIOError, so we can have this name reserved for future.
> But if you take the other way of letting this API to report IO errors
> for any device, I am perfectly comfortable with disk-only implementation
> for now.
> 

Oh, one more thing I thought is obvious but doesn't have to be for
others. In case you'll take the more generic way, let @dev be the alias
of the device which ought to be an unique string ID among the @dom.

Michal