[REVIEW] Storage mounting API

Jan Safranek jsafrane at redhat.com
Thu Mar 14 09:10:48 UTC 2013


On 03/13/2013 04:29 PM, Jan Synacek wrote:
> class LMI_Mount : CIM_Mount
> {
>     [ Description (
>         "Type of filesystem to create."),
>       ValueMap { "0", "2", "3", "4", "5", "6", "7", "8", "9", "10",
>                  "11", "12", "13", "..",
>                  "32769", "32770", "32771", "32772", "32773", "32768" },
>       Values { "Unknown", "UFS", "HFS", "FAT", "FAT16", "FAT32",
>                "NTFS4", "NTFS5", "XFS", "AFS", "EXT2", "EXT3",
>                "REISERFS", "DMTF Reserved",
>                "EXT4", "BTRFS", "JFS", "TMPFS", "VFAT", "OTHER"} ]
>     uint16 FileSystemType;
> 
>     [ Description (
>         "Specifies filesystem type other than listed in FileSystemType.") ]

Probably "Specifies filesystem type when FileSystemType property is set
to Other." And please use Other instead of OTHER, it's not an abbreviation.


> class LMI_MountSetting : CIM_SettingData
> {
>     [ Description (
>         "Other mount options that can be filesystem specific. This property is "
>         "also used to specify options with values (e.g. uid=0 or gid=100). "
>         "Other options are appended to the basic options."),
>     string OtherOptions[];

I would explicitly note that the mount options are used in the same
order as in this array. Also add ArrayType("Indexed") qualifier to
emphasize this.


> class LMI_MountService : CIM_Service
> {
>     uint32 CreateMount(
>         [ IN, Description (
>            "Existing filesystem that should be mounted.") ]
>         CIM_FileSystem REF FileSystem,
> 
>         [ IN, Description (
>             "Type of filesystem."),
>           ValueMap{ "0", "2", "3", "4", "5", "6", "7", "8", "9", "10",
>                     "11", "12", "13", "..",
>                     "32769", "32770", "32771", "32772", "32773", "32768" },
>           Values { "Unknown", "UFS", "HFS", "FAT", "FAT16", "FAT32",
>                    "NTFS4", "NTFS5", "XFS", "AFS", "EXT2", "EXT3",
>                    "REISERFS", "DMTF Reserved",
>                    "EXT4", "BTRFS", "JFS", "TMPFS", "VFAT", "OTHER"} ]
>         uint16 FileSystemType,

We already know the FS type from FileSystem parameter, FileSystemType
seems to be redundant.

> 
>         [ IN, Description (
>             "Directory where the mounted filesystem should be attached at.") ]
>         string MountPoint,
> 
>         [ IN, Description (
>             "Desired mount settings. If NULL, defaults will be used. Default "
>             "mount options are 'rw, suid, dev, exec, auto, nouser, async'.") ]
>         LMI_MountSettings REF Goal,
> 
>         [ IN(false), OUT, Description (
>             "Reference to the created job.") ]
>         CIM_ConcreteJob REF Job

There should be output parameter Mount with the created mount.

>     );
> 
>     [ Description (
>         "Modifies (remounts) an existing mount." ),
>       ValueMap { "0", "1", "2", "3", "4", "5", "6", "..", "4096",
>                  "4097", "4098..32767", "32768..65535" },
>       Values { "Job Completed with No Error", "Not Supported",
>                "Unknown", "Timeout", "Failed", "Invalid Parameter",
>                "In Use", "DMTF Reserved",
>                "Method Parameters Checked - Job Started",
>                "Size Not Supported", "Method Reserved", "Vendor Specific" } ]
>     uint32 ModifyMount(
>         [ IN, Description (
>             "An existing mount.") ]
>         LMI_Mount REF Mount,

Shouldn't it be also output parameter? That's how we do it in CIM.

> 
>         [ IN, Description (
>            "Desired mount settings. If NULL, defaults will be used. Default "
>            "mount options are 'rw, suid, dev, exec, auto, nouser, async'.") ]
>         LMI_MountSettings REF Goal,
> 
>         [ IN(false), OUT, Description (
>             "Reference to the created job.") ]
>         CIM_ConcreteJob REF Job
>     );

Otherwise it looks well, good job!

Jan


More information about the openlmi-devel mailing list