xfs
[Top] [All Lists]

Re: [PATCH 4/6] xfsprogs: libxcmd: isolate strdup() calls to fs_table_in

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 4/6] xfsprogs: libxcmd: isolate strdup() calls to fs_table_insert()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 6 Oct 2011 15:45:14 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <0f871eebf39384818415253082320f860739f113.1317646036.git.aelder@xxxxxxx>
References: <4a7a9e630aa7c62357a606f762abc19fc1d7073b.1317646036.git.aelder@xxxxxxx> <0f871eebf39384818415253082320f860739f113.1317646036.git.aelder@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
> @@ -95,21 +95,37 @@ fs_table_insert(
>  {
>       dev_t           datadev, logdev, rtdev;
>       struct fs_path  *tmp_fs_table;
> -
> -     if (!dir || !fsname)
> -             return EINVAL;
> +     int             error;
>  
>       datadev = logdev = rtdev = 0;
> -     if (fs_device_number(dir, &datadev))
> -             return errno;
> -     if (fslog && fs_device_number(fslog, &logdev))
> -             return errno;
> -     if (fsrt && fs_device_number(fsrt, &rtdev))
> -             return errno;
> +     error = fs_device_number(dir, &datadev);
> +     if (error)
> +             goto out_nodev;
> +     if (fslog && (error = fs_device_number(fslog, &logdev)))
> +             goto out_nodev;
> +     if (fsrt && (error = fs_device_number(fsrt, &rtdev)))
> +             goto out_nodev;

If you touch these anyway please move assignments outside the
conditionals, e.g.

         if (fslog) {
                error = fs_device_number(fslog, &logdev);
                if (error)
                        goto out_nodev;
        }

> -     char            *dir = NULL, *fsname = NULL;
>       int             error = 0;
>  
> -     if ((fs = fs_mount_point_from_path(udir)) != NULL) {
> -             dir = strdup(udir);
> -             fsname = strdup(fs->fs_name);
> -             if (dir && fsname)
> -                     error = fs_table_insert(dir, prid,
> -                                     FS_PROJECT_PATH, fsname, NULL, NULL);
> -             else
> -                     error = ENOMEM;
> +     if ((fs = fs_mount_point_from_path(dir)) != NULL) {

Same here.

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@xxxxxx>

<Prev in Thread] Current Thread [Next in Thread>