[Ocfs2-tools-devel] [PATCH 06/11] Ocfs2-tools: Add implementation of generic utils function to o2info.

[tristan]

Joel Becker wrote:
> On Fri, Jan 08, 2010 at 04:50:56PM +0800, Tristan Ye wrote:
>   
>> +errcode_t o2info_close(struct o2info_method *om)
>> +{
>> +	errcode_t err = 0;
>> +	int rc = 0;
>> +
>> +	if (om->om_method & O2INFO_USE_LIBOCFS2) {
>> +		if (om->om_fs) {
>> +			err = ocfs2_close(om->om_fs);
>> +			if (err) {
>> +				tcom_err(err, "while closing device");
>> +				goto out;
>> +			}
>> +		}
>> +	} else {
>> +		if (om->om_fd) {
>>     
>
> 	om_fd can be zero.  Check for om_fd<0.
>   

Oh, that's right, fd here can be zero theoretically...

>   
>> +int o2info_is_device(const char *path)
>> +{
>> +	int rc;
>> +	struct stat st;
>> +
>> +	rc = lstat(path, &st);
>>     
>
> 	You want stat() or even better fstat().  If you're looking at a
> symlink you actually want to follow it, I would think.  Most people
> would expect that.
>
>   

Thanks for the catching.


>> +	if ((S_ISBLK(st.st_mode)) || (S_ISCHR(st.st_mode)))
>> +		rc = 1;
>>     
>
> 	Remember, a symlink can't be BLK or CHR.
>
>   
>> +int o2info_uid2name(uid_t uid, char **uname)
>> +{
>> +	struct passwd *entry;
>> +	int ret = 0;
>> +
>> +	entry = getpwuid(uid);
>> +
>> +	if (!entry) {
>> +		errorf("user %d does not exist!\n", uid);
>> +		ret = -1;
>> +	} else {
>> +		*uname = strdup(entry->pw_name);
>> +		if (*uname == NULL) {
>> +			errorf("No memory for allocation\n");
>> +			ret = -1;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +int o2info_gid2name(gid_t gid, char **gname)
>> +{
>> +	struct group *group;
>> +	int ret = 0;
>> +
>> +	group = getgrgid(gid);
>> +
>> +	if (!group) {
>> +		errorf("group %d does not exist!\n", gid);
>> +		ret = -1;
>> +	} else {
>> +		*gname = strdup(group->gr_name);
>> +		if (*gname == NULL) {
>> +			errorf("No memory for allocation\n");
>> +			ret = -1;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>>     
>
> 	Here you're using strdup() instead of GString.  If you are going
> to use GLib, really use it.  Otherwise, drop it and use standard C
> library stuff.  Really, I'd be tempted to have the caller pass in a
> buffer, so as to avoid allocation.
>   

I copied some piece of codes from fsck.ocfs2 which was using gstring 
somewhere, yes, that may sound weird a little bit to use gstring and 
standard C string lib simultaneously there, I can do a unification anyway.

> 	I have to see how the utils are used in later functions, though,
> before I can provide more comments on the context.
>
> Joel
>
>