[libvirt] [PATCH 0/3] Fix io_timeout for sanlock
Just read the 3/3. I didn't know whether I should laugh or cry. I did both. Michal Privoznik (3): lock_driver_sanlock: Avoid global driver variable whenever possible m4: Check for sanlock_write_lockspace sanlock: Properly init io_timeout m4/virt-sanlock.m4 | 14 +++++--- src/locking/lock_driver_sanlock.c | 69 +++++++++++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 23 deletions(-) -- 2.8.4
Global variables are bad, we should avoid using them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 42 ++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 579f696..1ff1abd 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -80,7 +80,7 @@ struct _virLockManagerSanlockDriver { gid_t group; }; -static virLockManagerSanlockDriver *driver; +static virLockManagerSanlockDriverPtr sanlockDriver; struct _virLockManagerSanlockPrivate { const char *vm_uri; @@ -100,7 +100,8 @@ struct _virLockManagerSanlockPrivate { /* * sanlock plugin for the libvirt virLockManager API */ -static int virLockManagerSanlockLoadConfig(const char *configFile) +static int virLockManagerSanlockLoadConfig(virLockManagerSanlockDriverPtr driver, + const char *configFile) { virConfPtr conf; int ret = -1; @@ -161,7 +162,8 @@ static int virLockManagerSanlockLoadConfig(const char *configFile) /* How many times try adding a lockspace? */ #define LOCKSPACE_RETRIES 10 -static int virLockManagerSanlockSetupLockspace(void) +static int +virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) { int fd = -1; struct stat st; @@ -371,16 +373,20 @@ static int virLockManagerSanlockInit(unsigned int version, const char *configFile, unsigned int flags) { + virLockManagerSanlockDriverPtr driver; + VIR_DEBUG("version=%u configFile=%s flags=%x", version, NULLSTR(configFile), flags); virCheckFlags(0, -1); - if (driver) + if (sanlockDriver) return 0; - if (VIR_ALLOC(driver) < 0) + if (VIR_ALLOC(sanlockDriver) < 0) return -1; + driver = sanlockDriver; + driver->requireLeaseForDisks = true; driver->hostID = 0; driver->autoDiskLease = false; @@ -392,7 +398,7 @@ static int virLockManagerSanlockInit(unsigned int version, goto error; } - if (virLockManagerSanlockLoadConfig(configFile) < 0) + if (virLockManagerSanlockLoadConfig(driver, configFile) < 0) goto error; if (driver->autoDiskLease && !driver->hostID) { @@ -402,7 +408,7 @@ static int virLockManagerSanlockInit(unsigned int version, } if (driver->autoDiskLease) { - if (virLockManagerSanlockSetupLockspace() < 0) + if (virLockManagerSanlockSetupLockspace(driver) < -1) goto error; } @@ -415,11 +421,11 @@ static int virLockManagerSanlockInit(unsigned int version, static int virLockManagerSanlockDeinit(void) { - if (!driver) + if (!sanlockDriver) return 0; - VIR_FREE(driver->autoDiskLeasePath); - VIR_FREE(driver); + VIR_FREE(sanlockDriver->autoDiskLeasePath); + VIR_FREE(sanlockDriver); return 0; } @@ -438,7 +444,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1); - if (!driver) { + if (!sanlockDriver) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Sanlock plugin is not initialized")); return -1; @@ -566,7 +572,8 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock, -static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, +static int virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, + virLockManagerPtr lock, const char *name, size_t nparams, virLockManagerParamPtr params ATTRIBUTE_UNUSED, @@ -631,7 +638,8 @@ static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, } -static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) +static int virLockManagerSanlockCreateLease(virLockManagerSanlockDriverPtr driver, + struct sanlk_resource *res) { int fd = -1; struct stat st; @@ -719,6 +727,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerParamPtr params, unsigned int flags) { + virLockManagerSanlockDriverPtr driver = sanlockDriver; virLockManagerSanlockPrivatePtr priv = lock->privateData; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | @@ -738,11 +747,12 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { - if (virLockManagerSanlockAddDisk(lock, name, nparams, params, + if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params, !!(flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)) < 0) return -1; - if (virLockManagerSanlockCreateLease(priv->res_args[priv->res_count-1]) < 0) + if (virLockManagerSanlockCreateLease(driver, + priv->res_args[priv->res_count-1]) < 0) return -1; } else { if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | @@ -882,7 +892,7 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, if (priv->res_count == 0 && priv->hasRWDisks && - driver->requireLeaseForDisks) { + sanlockDriver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Read/write, exclusive access, disks were present, but no leases specified")); return -1; -- 2.8.4
On 09/15/2016 10:35 AM, Michal Privoznik wrote:
Global variables are bad, we should avoid using them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 42 ++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 579f696..1ff1abd 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -80,7 +80,7 @@ struct _virLockManagerSanlockDriver { gid_t group; };
-static virLockManagerSanlockDriver *driver; +static virLockManagerSanlockDriverPtr sanlockDriver;
struct _virLockManagerSanlockPrivate { const char *vm_uri; @@ -100,7 +100,8 @@ struct _virLockManagerSanlockPrivate { /* * sanlock plugin for the libvirt virLockManager API */ -static int virLockManagerSanlockLoadConfig(const char *configFile) +static int virLockManagerSanlockLoadConfig(virLockManagerSanlockDriverPtr driver, + const char *configFile)
static int virLock* Ironically you did this for the next one...
{ virConfPtr conf; int ret = -1; @@ -161,7 +162,8 @@ static int virLockManagerSanlockLoadConfig(const char *configFile) /* How many times try adding a lockspace? */ #define LOCKSPACE_RETRIES 10
-static int virLockManagerSanlockSetupLockspace(void) +static int +virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) { int fd = -1; struct stat st; @@ -371,16 +373,20 @@ static int virLockManagerSanlockInit(unsigned int version, const char *configFile, unsigned int flags) { + virLockManagerSanlockDriverPtr driver; + VIR_DEBUG("version=%u configFile=%s flags=%x", version, NULLSTR(configFile), flags); virCheckFlags(0, -1);
- if (driver) + if (sanlockDriver) return 0;
- if (VIR_ALLOC(driver) < 0) + if (VIR_ALLOC(sanlockDriver) < 0) return -1;
+ driver = sanlockDriver; + driver->requireLeaseForDisks = true; driver->hostID = 0; driver->autoDiskLease = false; @@ -392,7 +398,7 @@ static int virLockManagerSanlockInit(unsigned int version, goto error; }
- if (virLockManagerSanlockLoadConfig(configFile) < 0) + if (virLockManagerSanlockLoadConfig(driver, configFile) < 0) goto error;
if (driver->autoDiskLease && !driver->hostID) { @@ -402,7 +408,7 @@ static int virLockManagerSanlockInit(unsigned int version, }
if (driver->autoDiskLease) { - if (virLockManagerSanlockSetupLockspace() < 0) + if (virLockManagerSanlockSetupLockspace(driver) < -1) goto error; }
@@ -415,11 +421,11 @@ static int virLockManagerSanlockInit(unsigned int version,
static int virLockManagerSanlockDeinit(void) { - if (!driver) + if (!sanlockDriver) return 0;
- VIR_FREE(driver->autoDiskLeasePath); - VIR_FREE(driver); + VIR_FREE(sanlockDriver->autoDiskLeasePath); + VIR_FREE(sanlockDriver);
return 0; } @@ -438,7 +444,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1);
- if (!driver) { + if (!sanlockDriver) {
Interesting that this is the one API which checks - every other API assumes sanlockDriver would be set since they can assume the Init function was run.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Sanlock plugin is not initialized")); return -1; @@ -566,7 +572,8 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock,
-static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, +static int virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, + virLockManagerPtr lock,
static int virLock*
const char *name, size_t nparams, virLockManagerParamPtr params ATTRIBUTE_UNUSED, @@ -631,7 +638,8 @@ static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, }
-static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) +static int virLockManagerSanlockCreateLease(virLockManagerSanlockDriverPtr driver, +
static int virLock* struct sanlk_resource *res)
{ int fd = -1; struct stat st; @@ -719,6 +727,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerParamPtr params, unsigned int flags) { + virLockManagerSanlockDriverPtr driver = sanlockDriver;
Existing - unlike virLockManagerSanlockNew this code doesn't check for !sanlockDriver (or !driver) and fail...
virLockManagerSanlockPrivatePtr priv = lock->privateData;
virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | @@ -738,11 +747,12 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { - if (virLockManagerSanlockAddDisk(lock, name, nparams, params, + if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params, !!(flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)) < 0) return -1;
- if (virLockManagerSanlockCreateLease(priv->res_args[priv->res_count-1]) < 0) + if (virLockManagerSanlockCreateLease(driver, + priv->res_args[priv->res_count-1]) < 0) return -1; } else { if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | @@ -882,7 +892,7 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
You went away from your model... virLockManagerSanlockDriverPtr driver = sanlockDriver; and use driver... it's also of note that there is not a !driver check here too. In any case, the issues are cosmetic and your choice on how to handle. ACK John
if (priv->res_count == 0 && priv->hasRWDisks && - driver->requireLeaseForDisks) { + sanlockDriver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Read/write, exclusive access, disks were present, but no leases specified")); return -1;
Currently, we are checking for sanlock_add_lockspace_timeout which is good for now. But in the next patches we are gonna use sanlock_write_lockspace (which sets an initial value for io timeout for sanlock). Now, there is no reason to check for both functions in sanlock library as the sanlock_write_lockspace was introduced in 2.7 release and the one we are currently checking for in the 2.5 release. Therefore it is safe to assume presence of sanlock_add_lockspace_timeout when sanlock_write_lockspace is detected. Moreover, the macro for conditional compilation is renamed to HAVE_SANLOCK_IO_TIMEOUT (as it now encapsulates two functions). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-sanlock.m4 | 14 +++++++++----- src/locking/lock_driver_sanlock.c | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/m4/virt-sanlock.m4 b/m4/virt-sanlock.m4 index d2a607d..3c30cbf 100644 --- a/m4/virt-sanlock.m4 +++ b/m4/virt-sanlock.m4 @@ -46,11 +46,15 @@ AC_DEFUN([LIBVIRT_CHECK_SANLOCK],[ [whether sanlock supports sanlock_inq_lockspace]) fi - AC_CHECK_LIB([sanlock_client], [sanlock_add_lockspace_timeout], - [sanlock_add_lockspace_timeout=yes], [sanlock_add_lockspace_timeout=no]) - if test "x$sanlock_add_lockspace_timeout" = "xyes" ; then - AC_DEFINE_UNQUOTED([HAVE_SANLOCK_ADD_LOCKSPACE_TIMEOUT], 1, - [whether Sanlock supports sanlock_add_lockspace_timeout]) + dnl Ideally, we would check for sanlock_add_lockspace_timeout here too, but + dnl sanlock_write_lockspace has been introduced 2 releases after + dnl sanlock_add_lockspace_timeout therefore if sanlock_write_lockspace is found + dnl it is safe to assume sanlock_add_lockspace_timeout is present too. + AC_CHECK_LIB([sanlock_client], [sanlock_write_lockspace], + [sanlock_write_lockspace=yes], [sanlock_write_lockspace=no]) + if test "x$sanlock_write_lockspace" = "xyes" ; then + AC_DEFINE_UNQUOTED([HAVE_SANLOCK_IO_TIMEOUT], 1, + [whether sanlock supports sanlock_write_lockspace]) fi CPPFLAGS="$old_cppflags" diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 1ff1abd..f09027f 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -310,7 +310,7 @@ virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) * or we can fallback to polling. */ retry: -#ifdef HAVE_SANLOCK_ADD_LOCKSPACE_TIMEOUT +#ifdef HAVE_SANLOCK_IO_TIMEOUT rv = sanlock_add_lockspace_timeout(&ls, 0, driver->io_timeout); #else if (driver->io_timeout) { -- 2.8.4
On 09/15/2016 10:35 AM, Michal Privoznik wrote:
Currently, we are checking for sanlock_add_lockspace_timeout which is good for now. But in the next patches we are gonna use
s/the next patches/a subsequent patch/ s/gonna/going to/
sanlock_write_lockspace (which sets an initial value for io timeout for sanlock). Now, there is no reason to check for both functions in sanlock library as the sanlock_write_lockspace was introduced in 2.7 release and the one we are currently checking for in the 2.5 release. Therefore it is safe to assume presence of sanlock_add_lockspace_timeout when sanlock_write_lockspace is detected.
Moreover, the macro for conditional compilation is renamed to HAVE_SANLOCK_IO_TIMEOUT (as it now encapsulates two functions).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-sanlock.m4 | 14 +++++++++----- src/locking/lock_driver_sanlock.c | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-)
ACK (seems reasonable to me, but my m4 knowledge is light) John
https://bugzilla.redhat.com/show_bug.cgi?id=1292984 Hold on to your hats, because this is gonna be wild. In bd3e16a3 I've tried to expose sanlock io_timeout. What I had not realized (because there is like no documentation for sanlock at all) was very unusual way their APIs work. Basically, what we do currently is: sanlock_add_lockspace_timeout(&ls, io_timeout); which adds a lockspace to sanlock daemon. One would expect that io_timeout sets the io_timeout for it. Nah! That's where you are completely off the tracks. It sets timeout for next lockspace you will probably add later. Therefore: sanlock_add_lockspace_timeout(&ls, io_timeout = 10); /* adds new lockspace with default io_timeout */ sanlock_add_lockspace_timeout(&ls, io_timeout = 20); /* adds new lockspace with io_timeout = 10 */ sanlock_add_lockspace_timeout(&ls, io_timeout = 40); /* adds new lockspace with io_timeout = 20 */ And so on. You get the picture. Fortunately, we don't allow setting io_timeout per domain or per domain disk. So we just need to set the default used in the very first step and hope for the best (as all the io_timeout-s used later will have the same value). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index f09027f..ee1ee67 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -157,6 +157,29 @@ static int virLockManagerSanlockLoadConfig(virLockManagerSanlockDriverPtr driver return ret; } +static int +virLockManagerSanlockInitLockspace(virLockManagerSanlockDriverPtr driver, + struct sanlk_lockspace *ls) +{ + int ret; + +#ifdef HAVE_SANLOCK_IO_TIMEOUT + const int max_hosts = 0; /* defaults used in sanlock_init() implementation */ + const unsigned int lockspaceFlags = 0; + + ret = sanlock_write_lockspace(ls, max_hosts, lockspaceFlags, driver->io_timeout); +#else + if (driver->io_timeout) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unable to use io_timeout with this version of sanlock")); + return -ENOTSUP; + } + + ret = sanlock_init(ls, NULL, 0, 0); +#endif + return ret; +} + /* How much ms sleep before retrying to add a lockspace? */ #define LOCKSPACE_SLEEP 100 /* How many times try adding a lockspace? */ @@ -267,7 +290,7 @@ virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver) goto error_unlink; } - if ((rv = sanlock_init(&ls, NULL, 0, 0)) < 0) { + if ((rv = virLockManagerSanlockInitLockspace(driver, &ls) < 0)) { if (rv <= -200) virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to initialize lockspace %s: error %d"), -- 2.8.4
On 09/15/2016 10:35 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1292984
Hold on to your hats, because this is gonna be wild.
In bd3e16a3 I've tried to expose sanlock io_timeout. What I had not realized (because there is like no documentation for sanlock at all) was very unusual way their APIs work. Basically, what we do currently is:
sanlock_add_lockspace_timeout(&ls, io_timeout);
which adds a lockspace to sanlock daemon. One would expect that io_timeout sets the io_timeout for it. Nah! That's where you are completely off the tracks. It sets timeout for next lockspace you will probably add later. Therefore:
sanlock_add_lockspace_timeout(&ls, io_timeout = 10); /* adds new lockspace with default io_timeout */
sanlock_add_lockspace_timeout(&ls, io_timeout = 20); /* adds new lockspace with io_timeout = 10 */
sanlock_add_lockspace_timeout(&ls, io_timeout = 40); /* adds new lockspace with io_timeout = 20 */
And so on. You get the picture. Fortunately, we don't allow setting io_timeout per domain or per domain disk. So we just need to set the default used in the very first step and hope for the best (as all the io_timeout-s used later will have the same value).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
Any thoughts about modifying src/locking/sanlock.conf in order add some text there that indicates support for 'io_timeout' requires at least sanlock 2.7 (or something similar). Beyond that things seem reasonable to me ACK John
On 28.09.2016 23:59, John Ferlan wrote:
On 09/15/2016 10:35 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1292984
Hold on to your hats, because this is gonna be wild.
In bd3e16a3 I've tried to expose sanlock io_timeout. What I had not realized (because there is like no documentation for sanlock at all) was very unusual way their APIs work. Basically, what we do currently is:
sanlock_add_lockspace_timeout(&ls, io_timeout);
which adds a lockspace to sanlock daemon. One would expect that io_timeout sets the io_timeout for it. Nah! That's where you are completely off the tracks. It sets timeout for next lockspace you will probably add later. Therefore:
sanlock_add_lockspace_timeout(&ls, io_timeout = 10); /* adds new lockspace with default io_timeout */
sanlock_add_lockspace_timeout(&ls, io_timeout = 20); /* adds new lockspace with io_timeout = 10 */
sanlock_add_lockspace_timeout(&ls, io_timeout = 40); /* adds new lockspace with io_timeout = 20 */
And so on. You get the picture. Fortunately, we don't allow setting io_timeout per domain or per domain disk. So we just need to set the default used in the very first step and hope for the best (as all the io_timeout-s used later will have the same value).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
Any thoughts about modifying src/locking/sanlock.conf in order add some text there that indicates support for 'io_timeout' requires at least sanlock 2.7 (or something similar).
Huh, well, versions are tricky a bit. Somebody might just backport stuff and all of a sudden this might work with sanlock-2.6 too. I think we just avoid giving these kind of advice. Michal
On 15.09.2016 16:35, Michal Privoznik wrote:
Just read the 3/3. I didn't know whether I should laugh or cry. I did both.
Michal Privoznik (3): lock_driver_sanlock: Avoid global driver variable whenever possible m4: Check for sanlock_write_lockspace sanlock: Properly init io_timeout
m4/virt-sanlock.m4 | 14 +++++--- src/locking/lock_driver_sanlock.c | 69 +++++++++++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 23 deletions(-)
Thanks John for ACKing the series. I'm not quite sure whether I can push it now that we are in the freeze. I mean it can be viewed as a bug fix, so I'm inclined to push it. What are your thoughts? Michal
On 09/29/2016 10:06 AM, Michal Privoznik wrote:
On 15.09.2016 16:35, Michal Privoznik wrote:
Just read the 3/3. I didn't know whether I should laugh or cry. I did both.
Michal Privoznik (3): lock_driver_sanlock: Avoid global driver variable whenever possible m4: Check for sanlock_write_lockspace sanlock: Properly init io_timeout
m4/virt-sanlock.m4 | 14 +++++--- src/locking/lock_driver_sanlock.c | 69 +++++++++++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 23 deletions(-)
Thanks John for ACKing the series. I'm not quite sure whether I can push it now that we are in the freeze. I mean it can be viewed as a bug fix, so I'm inclined to push it. What are your thoughts?
From the aspect of you have a legitimate bug that's causing a feature to not work properly, sure I agree. From the aspect of I've pushed something during a freeze before and was told I shouldn't have, maybe I'm not the best person to ask ;-).
BTW: I understand your point about not wanting to document a specific version since it's possible (I suppose) that the API could be backported to some earlier release (not that we'd ever do that). Still, I'm reacting to a feature someone may have thought was working that suddenly will cause a failure to start a domain if they don't have the new API, but did have the old API. At least a running guest won't "go missing". IOW: how can we inform the user that the minimum version we expected now changed because of some change in an underlying package we depend on. All that said - push and say sorry later ;-) John
On 29.09.2016 16:50, John Ferlan wrote:
On 09/29/2016 10:06 AM, Michal Privoznik wrote:
On 15.09.2016 16:35, Michal Privoznik wrote:
Just read the 3/3. I didn't know whether I should laugh or cry. I did both.
Michal Privoznik (3): lock_driver_sanlock: Avoid global driver variable whenever possible m4: Check for sanlock_write_lockspace sanlock: Properly init io_timeout
m4/virt-sanlock.m4 | 14 +++++--- src/locking/lock_driver_sanlock.c | 69 +++++++++++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 23 deletions(-)
Thanks John for ACKing the series. I'm not quite sure whether I can push it now that we are in the freeze. I mean it can be viewed as a bug fix, so I'm inclined to push it. What are your thoughts?
From the aspect of you have a legitimate bug that's causing a feature to not work properly, sure I agree. From the aspect of I've pushed something during a freeze before and was told I shouldn't have, maybe I'm not the best person to ask ;-).
BTW: I understand your point about not wanting to document a specific version since it's possible (I suppose) that the API could be backported to some earlier release (not that we'd ever do that).
Still, I'm reacting to a feature someone may have thought was working that suddenly will cause a failure to start a domain if they don't have the new API, but did have the old API. At least a running guest won't "go missing". IOW: how can we inform the user that the minimum version we expected now changed because of some change in an underlying package we depend on.
Well, I guess they will find out as soon they try to start a domain. If they are running old sanlock (which they shouldn't at all - we advocate for using our virlockd), they'll see a sensible error message: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unable to use io_timeout with this version of sanlock"));
All that said - push and say sorry later ;-)
Done :-) Thank you. Michal
John
participants (2)
-
John Ferlan -
Michal Privoznik