[Libstoragemgmt-devel] Error number review

Tony Asleson

2014-08-06 00:02:30 UTC

Post by Gris Ge
Hi Guys,
https://sourceforge.net/p/libstoragemgmt/wiki/v1_py_api_user_guide/#appendixf-exceptions-lsmlsmerror
=========
1. JOB_STARTED
Not used by any plugin or lsm.Client methods.
Found in lsmcli where it use it as sys.exit().

It's used in two places in the command line (line 1256 & 1229).

Post by Gris Ge
Remove it from lsm and move to lsmcli.

This is for the lsmcli exit value when a job is started. This is also
used in the C code as an exit value from any of the C functions to
indicate a job has started. It may not be used much in python as we use
a tuple (job, item), but it is retained in the python constants so that
they are consistent with the C constants and so that is doesn't
incorrectly get re-used. We need to leave this as is.

Post by Gris Ge
2. INDEX_BOUNDS
No idea what it means.
Need documnet.

This is the common error code when using a string list with an invalid
array index. Patch supplied that changes this to INVALID_ARGUMENT.

Post by Gris Ge
3. EXISTS_ACCESS_GROUP and other EXISTS_XXX
SMI-S plugin is using EXISTS_ACCESS_GROUP to indicate
another access group is using requested name but not containing
requested initiator ID.
User cannot quickly get this long explaination simply from error
number name, and this is not a sharing error number.
Should we support the replay call -- user invoke the same method
with same argument 2 or more times?
Currently, access group does not raise error on replay call.
a. Might be harmful.
If pass the replay call, user might incorrectly use pre-exist
volume. Same thing also apply to fs creation.
We should raise CONFLICT_NAME for this kind of reply call.
b. No harm.
* volume-mask.
* access_group_create()
# with pre-exist access group containing only requested
# initiator id and using request access group name.
Unifiy them into CONFLICT_NAME indicate request name is already in
use when creating.
Another new error number would be CONFLICT_MEMBER indicate requested
member is already used by others. For example, access_group_create()
got a initiator which already used by other access group.

Should the API be idempotent, that is the question. I'm trying to think
if there are cases where this would result in a bad outcome? Double
create/delete/re-size etc. Do you report success on a volume create if
the volume name already exists or do we fail? I'm leaning towards
continuing with the design that calls are not idempotent.

I think your suggestion seems reasonable.

Post by Gris Ge
4. FS_NOT_EXPORTED
No idea what it means.
Need documnet.

This indicates a file system is not exported and you are trying to
un-export it. This is in the same context as above WRT idempotent

Post by Gris Ge
5. INDEX_BOUNDS
Used today in one case of of StringList when the programmer over
indexes the list. This could be a generic INVALID_ARGUMENT error.
Rename to INVALID_ARGUMENT

Done

Post by Gris Ge
6. INVALID_XXX
Most of these are magic check error codes for the C API which
violates #4 item above as all of these are programming errors, not
user input errors. All of these should probably be changed to
INVALID_ARGUMENT as the programmer can fix during development.
Replace by INVALID_ARGUMENT

Done

Post by Gris Ge
7. IS_MAPPED and NO_MAPPING
Whether we should treat this no-harm replay call as failure or pass.
Treat no-harm replay call as pass.

See above on idempotent.

Post by Gris Ge
8. NO_CONNECT
I believe this error should be used to indicate that the plug-in
cannot connect to the array. Only targetd is using it, but I was
unable to force the error condition in my limited testing.
Base on targetd code, it should be replaced by NETWORK_ERROR or
NETWORK_HOSTDOWN.

Converted to NETWORK_ERROR

Post by Gris Ge
9. NOT_FOUND_SS
Might confused with volume snapshot.
Renamed to NOT_FOUND_FS_SNAP

Renamed to NOT_FOUND_FS_SS

Post by Gris Ge
10. NOT_FOUND_NFS_EXPORT
In LSM, we have lsm.NfsExport, export_fs(), exports() and
NOT_FOUND_NFS_EXPORT,
fs_export() # like volume_mask()
fs_export_infos() # like planed mask_infos()
NOT_FOUND_FS_EXPORT
lsm.FsExport

These are referring to similar, but different things. I think keeping
the terminology of fs export is better than making it sound like a
logical volume operation.

Post by Gris Ge
11. NOT_FOUND_DISK
No method require this yet.
Remove.

This being used for pool_create_from_disks in the C simulator. We
remove pool create stuff then we can remove this.

Post by Gris Ge
12. NOT_IMPLEMENTED
Does this one mean storage SDK/array can do, but plugin is coded yet.
Use NO_SUPPORT instead.

Done

Post by Gris Ge
13. OFFLINE and ON_LINE
Some actions require resource be online or offline before invoking
methods. But literally, OFFLINE might indicate plugin or storage array
or resource offline.
Replace by NO_SUPPORT_ONLINE_CHANGE and NO_SUPPORT_OFFLINE_CHANGE.
And provide dedicate capabilities to guide user.
Which exactly method require this?

Done

Post by Gris Ge
14. PLUGIN_DLOPEN
Need to change name, this error indicted C API was unable to
establish, RPC socket to plug-in
SOCKET_TRANS_FAIL
Along with SOCKET_SERIALIZATER_FAIL and SOCKET_JSON_INVALID_ARG

Changed to PLUGIN_IPC_FAIL that it's clear that the IPC is failing not
the socket from the plugin to the array etc.

Post by Gris Ge
15. PLUGIN_DLSYM
Concerns
Can be deleted, was used when we had shared object plugins
Remove.

Done

Post by Gris Ge
16. PLUGIN_ERROR
Should be replaced by LSM_PLUGIN_BUG
Replace by LSM_PLUGIN_BUG

Done

Post by Gris Ge
17. PLUGIN_MISSING_HOST, PLUGIN_MISSING_NS, PLUGIN_MISSING_PORT
Can be replaced by INVALID_URI
Replace by INVALID_URI for URI syntax error and require field
missing.

Replaced with INVALID_ARGUMENT. Above you advocated no more INVALID_*.

Post by Gris Ge
18. PLUGIN_PERMISSION
Renamed to PLUGIN_SOCKET_PERMISSION_ERROR
Check above

Changed to PLUGIN_SOCKET_PERMISSION, the error part is implied.

Post by Gris Ge
19. PLUGIN_UNKNOWN_HOST
DNS name resolve problem could be count as NETWORK_ERROR
Replace by NETWORK_ERROR

This wasn't being used, deleted.

Post by Gris Ge
20. PLUGIN_TIMEOUT
Duplicate with TIMEOUT
Remove.

Done

Post by Gris Ge
21. SIZE_INSUFFICIENT_SPACE, SIZE_TOO_LARGE
Replace by NO_ENOUGH_SPACE
Suggest.
Replace by NO_ENOUGH_SPACE

Done

Post by Gris Ge
22. SIZE_SAME
If size is the same, volume_resize() or fs_resize() should treat it
as pass.
Remove.

This falls under if calls should be idempotent in my opinion.

Post by Gris Ge
23. SIZE_TOO_SMALL
Any ideas? I(Gris) haven't try to reproduce it yet.
We keep it.
24. UNSUPPORTED_PROVISIONING
Replace with INVALID_ARGUMENT or NO_SUPPORT base on use case.

Done

Post by Gris Ge
25. DISK_BUSY
No need.
Remove.

We are using this in the simulator for pool create. Remove when pool
create calls are removed.

Post by Gris Ge
26. VOLUME_BUSY
VOLUME_MASKED
VOLUME_IS_REPLICATE_SOURCE # Are we keeping volume_replicate()?

Yes, I would like to keep replicate.

Post by Gris Ge
27. ACCESS_GROUP_BUSY and MASKED_ACCESS_GROUP
ACCESS_GROUP_MASKED
=========

Done

Thanks,
Tony