Re: [REVIEW] Storage mounting API

[Jan Safranek]

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