On 09/03/2018 11:13 AM, Michal Privoznik wrote:
> On 08/30/2018 11:34 PM, John Ferlan wrote:
>>
>>
>> On 08/27/2018 04:08 AM, Michal Privoznik wrote:
>>> This is a new type of object that lock drivers can handle.
>>> Currently, it is supported by lockd driver only.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>>> ---
>>> src/locking/lock_driver.h | 2 ++
>>> src/locking/lock_driver_lockd.c | 43 +++++++++++++++++++++++++++++++--------
>>> src/locking/lock_driver_sanlock.c | 3 ++-
>>> 3 files changed, 38 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
>>> index a9d2041c30..9be0abcfba 100644
>>> --- a/src/locking/lock_driver.h
>>> +++ b/src/locking/lock_driver.h
>>> @@ -51,6 +51,8 @@ typedef enum {
>>> VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0,
>>> /* A lease against an arbitrary resource */
>>> VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE = 1,
>>> + /* The resource to be locked is a metadata */
>>> + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA = 2,
>>> } virLockManagerResourceType;
>>>
>>> typedef enum {
>>> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
>>> index 98953500b7..d7cb183d7a 100644
>>> --- a/src/locking/lock_driver_lockd.c
>>> +++ b/src/locking/lock_driver_lockd.c
>>> @@ -557,6 +557,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>>> virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
>>> char *newName = NULL;
>>> char *newLockspace = NULL;
>>> + int newFlags = 0;
>>> bool autoCreate = false;
>>> int ret = -1;
>>>
>>> @@ -569,7 +570,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>>> switch (priv->type) {
>>> case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
>>>
>>> - switch (type) {
>>> + switch ((virLockManagerResourceType) type) {
>>> case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>>> if (params || nparams) {
>>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> @@ -670,6 +671,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>>> goto cleanup;
>>>
>>> } break;
>>> +
>>> + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:
>>
>> I'm still conflicted with Unknown and Unsupported.
>>
>>> default:
>
> As I explain in one of my previous replies, users are not really
> expected to see this message. Is merely for us to avoid broken code
> pattern. Even if it so happens that broken code slips through review,
> what difference does it make for users to see "Unsupported lock manager
> object type" vs "Unknown lock manager object type"? They'll file a bug
> and we will notice immediately what is the problem when looking into the
> code (we will notice it because the error message logs type number).
>
Consider my your early warning bz then ;-). It doesn't really matter
that much... Maybe it's more of an "Unsupported" message regardless of
whether it's META or 'default'. At first I considered noting the Enum
range error, but it's not the same here.
I'll leave it as a design decision and move on.
John
>>> virReportError(VIR_ERR_INTERNAL_ERROR,
>>> _("Unknown lock manager object type %d for domain lock object"),
>>> @@ -679,6 +682,29 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>>> break;
>>>
>>> case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
>>> + switch ((virLockManagerResourceType) type) {
>>> + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:
>>> + if (params || nparams) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> + _("Unexpected parameters for metadata resource"));
>>> + goto cleanup;
>>> + }
>>> + if (VIR_STRDUP(newLockspace, "") < 0 ||
>>> + VIR_STRDUP(newName, name) < 0)
>>> + goto cleanup;
>>> + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA;
>>> + break;
>>> +
>>> + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>>> + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE:
>>
>> Again Unknown and Unsupported...
>>
>>> + default:
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("Unknown lock manager object type %d for daemon lock object"),
>>> + type);
>>> + goto cleanup;
>>> + }
>>> + break;
>>> +
>>> default:
>>> virReportError(VIR_ERR_INTERNAL_ERROR,
>>> _("Unknown lock manager object type %d"),
>>> @@ -686,19 +712,18 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>>> goto cleanup;
>>> }
>>>
>>> + if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)
>>> + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED;
>>> +
>>> + if (autoCreate)
>>
>> Interstingly enough, @newFlags is adjusted in the new case and we could
>> do the same in the existing case instead of setting @autoCreate, just
>> set the newFlags. Of course I'm quite aware that this could have been
>> done in a separate patch too. IOW: I could easily support removing
>> @autoCreate...
>
> Okay.
>
> Michal
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
