[libvirt] [PATCH] sanlock: Truncate domain names longer than SANLK_NAME_LEN
Libvirt uses a domain name to fill in owner_name in sanlock_options in virLockManagerSanlockAcquire. Unfortunately, owner_name is limited to SANLK_NAME_LEN characters (including trailing '\0'), which means domains with longer names fail to start when sanlock is enabled. However, we can truncate the name when setting owner_name as explained by sanlock's author: Setting sanlk_options or the owner_name is unnecessary, and has very little to no benefit. If you do provide something in owner_name, it can be anything, sanlock doesn't care or use it. If you run the command "sanlock status", the output will display a list of clients connected to the sanlock daemon. This client list is displayed as "pid owner_name" if the client has provided an owner_name via sanlk_options. This debugging output is the only usage of owner_name, so its only benefit is to potentially provide a more human friendly output for debugging purposes. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/locking/lock_driver_sanlock.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 958d39a..f11f3c6 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -81,7 +81,7 @@ static virLockManagerSanlockDriver *driver = NULL; struct _virLockManagerSanlockPrivate { const char *vm_uri; - char vm_name[SANLK_NAME_LEN]; + char *vm_name; unsigned char vm_uuid[VIR_UUID_BUFLEN]; unsigned int vm_id; unsigned int vm_pid; @@ -474,12 +474,8 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, if (STREQ(param->key, "uuid")) { memcpy(priv->vm_uuid, param->value.uuid, 16); } else if (STREQ(param->key, "name")) { - if (!virStrcpy(priv->vm_name, param->value.str, SANLK_NAME_LEN)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain name '%s' exceeded %d characters"), - param->value.str, SANLK_NAME_LEN); + if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) goto error; - } } else if (STREQ(param->key, "pid")) { priv->vm_pid = param->value.ui; } else if (STREQ(param->key, "id")) { @@ -505,6 +501,7 @@ static void virLockManagerSanlockFree(virLockManagerPtr lock) if (!priv) return; + VIR_FREE(priv->vm_name); for (i = 0; i < priv->res_count; i++) VIR_FREE(priv->res_args[i]); VIR_FREE(priv); @@ -909,12 +906,10 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, if (VIR_ALLOC(opt) < 0) return -1; - if (!virStrcpy(opt->owner_name, priv->vm_name, SANLK_NAME_LEN)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain name '%s' exceeded %d characters"), - priv->vm_name, SANLK_NAME_LEN); - goto error; - } + /* sanlock doesn't use owner_name for anything, so it's safe to take just + * the first SANLK_NAME_LEN - 1 characters from vm_name */ + ignore_value(virStrncpy(opt->owner_name, priv->vm_name, + SANLK_NAME_LEN - 1, SANLK_NAME_LEN)); if (state && STRNEQ(state, "")) { if ((rv = sanlock_state_to_args((char *)state, -- 1.9.0
On Thu, Feb 27, 2014 at 09:43:10AM +0100, Jiri Denemark wrote:
Libvirt uses a domain name to fill in owner_name in sanlock_options in virLockManagerSanlockAcquire. Unfortunately, owner_name is limited to SANLK_NAME_LEN characters (including trailing '\0'), which means domains with longer names fail to start when sanlock is enabled. However, we can truncate the name when setting owner_name as explained by sanlock's author:
Setting sanlk_options or the owner_name is unnecessary, and has very little to no benefit. If you do provide something in owner_name, it can be anything, sanlock doesn't care or use it.
If you run the command "sanlock status", the output will display a list of clients connected to the sanlock daemon. This client list is displayed as "pid owner_name" if the client has provided an owner_name via sanlk_options. This debugging output is the only usage of owner_name, so its only benefit is to potentially provide a more human friendly output for debugging purposes.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/locking/lock_driver_sanlock.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 958d39a..f11f3c6 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -81,7 +81,7 @@ static virLockManagerSanlockDriver *driver = NULL;
struct _virLockManagerSanlockPrivate { const char *vm_uri; - char vm_name[SANLK_NAME_LEN]; + char *vm_name; unsigned char vm_uuid[VIR_UUID_BUFLEN]; unsigned int vm_id; unsigned int vm_pid; @@ -474,12 +474,8 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, if (STREQ(param->key, "uuid")) { memcpy(priv->vm_uuid, param->value.uuid, 16); } else if (STREQ(param->key, "name")) { - if (!virStrcpy(priv->vm_name, param->value.str, SANLK_NAME_LEN)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain name '%s' exceeded %d characters"), - param->value.str, SANLK_NAME_LEN); + if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) goto error; - } } else if (STREQ(param->key, "pid")) { priv->vm_pid = param->value.ui; } else if (STREQ(param->key, "id")) { @@ -505,6 +501,7 @@ static void virLockManagerSanlockFree(virLockManagerPtr lock) if (!priv) return;
+ VIR_FREE(priv->vm_name); for (i = 0; i < priv->res_count; i++) VIR_FREE(priv->res_args[i]); VIR_FREE(priv); @@ -909,12 +906,10 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, if (VIR_ALLOC(opt) < 0) return -1;
- if (!virStrcpy(opt->owner_name, priv->vm_name, SANLK_NAME_LEN)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain name '%s' exceeded %d characters"), - priv->vm_name, SANLK_NAME_LEN); - goto error; - } + /* sanlock doesn't use owner_name for anything, so it's safe to take just + * the first SANLK_NAME_LEN - 1 characters from vm_name */ + ignore_value(virStrncpy(opt->owner_name, priv->vm_name, + SANLK_NAME_LEN - 1, SANLK_NAME_LEN));
if (state && STRNEQ(state, "")) { if ((rv = sanlock_state_to_args((char *)state,
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/27/2014 09:43 AM, Jiri Denemark wrote:
Libvirt uses a domain name to fill in owner_name in sanlock_options in virLockManagerSanlockAcquire. Unfortunately, owner_name is limited to SANLK_NAME_LEN characters (including trailing '\0'), which means domains with longer names fail to start when sanlock is enabled. However, we can truncate the name when setting owner_name as explained by sanlock's author:
Setting sanlk_options or the owner_name is unnecessary, and has very little to no benefit. If you do provide something in owner_name, it can be anything, sanlock doesn't care or use it.
If you run the command "sanlock status", the output will display a list of clients connected to the sanlock daemon. This client list is displayed as "pid owner_name" if the client has provided an owner_name via sanlk_options. This debugging output is the only usage of owner_name, so its only benefit is to potentially provide a more human friendly output for debugging purposes.
It might be nice to add a link the bug: https://bugzilla.redhat.com/show_bug.cgi?id=1060557 Jan
On Thu, Feb 27, 2014 at 11:33:50 +0100, Jano Tomko wrote:
On 02/27/2014 09:43 AM, Jiri Denemark wrote:
Libvirt uses a domain name to fill in owner_name in sanlock_options in virLockManagerSanlockAcquire. Unfortunately, owner_name is limited to SANLK_NAME_LEN characters (including trailing '\0'), which means domains with longer names fail to start when sanlock is enabled. However, we can truncate the name when setting owner_name as explained by sanlock's author:
Setting sanlk_options or the owner_name is unnecessary, and has very little to no benefit. If you do provide something in owner_name, it can be anything, sanlock doesn't care or use it.
If you run the command "sanlock status", the output will display a list of clients connected to the sanlock daemon. This client list is displayed as "pid owner_name" if the client has provided an owner_name via sanlk_options. This debugging output is the only usage of owner_name, so its only benefit is to potentially provide a more human friendly output for debugging purposes.
It might be nice to add a link the bug:
Oops, too late, I just pushed the patch. Jirka
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Ján Tomko