[mdadm PATCH 1/1] Fix a build error

[mdadm PATCH 1/1] Fix a build error

[Xiao Ni]

On the s390 platform the build fails with the error below.
Manage.c: In function 'Manage_subdevs':
Manage.c:1502:5: error: passing argument 3 of 'fstat_is_blkdev' from incompatible pointer type [-Werror]
     fstat_is_blkdev(tfd, dv->devname, &rdev);
     ^
In file included from Manage.c:25:0:
mdadm.h:1446:12: note: expected 'dev_t *' but argument is of type 'long unsigned int *'

It was introduced by commit 0a6bff09 (mdadm/util: unify fstat
checking blkdev into function). It needs to pass a type 'dev_t'
argument to fstat_is_blkdev, but it passes a type 'unsigned
long' argument. So use a temporary variable to fix this.

Signed-off-by: Xiao Ni <xni@redhat.com>
Suggested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Suggested-by: Wols Lists <antlists@youngman.org.uk>
---
 Manage.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Manage.c b/Manage.c
index 871d342..6de1fc2 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1497,13 +1497,14 @@ int Manage_subdevs(char *devname, int fd,
 			 */
 			rdev = makedev(mj, mn);
 		} else {
+			dev_t device_id;
 			tfd = dev_open(dv->devname, O_RDONLY);
 			if (tfd >= 0) {
-				fstat_is_blkdev(tfd, dv->devname, &rdev);
+				fstat_is_blkdev(tfd, dv->devname, &device_id);
 				close(tfd);
 			} else {
 				int open_err = errno;
-				if (!stat_is_blkdev(dv->devname, &rdev)) {
+				if (!stat_is_blkdev(dv->devname, &device_id)) {
 					if (dv->disposition == 'M')
 						/* non-fatal. Also improbable */
 						continue;
@@ -1523,6 +1524,7 @@ int Manage_subdevs(char *devname, int fd,
 					goto abort;
 				}
 			}
+			rdev = (unsigned long)device_id;
 		}
 		switch(dv->disposition){
 		default:
-- 
2.7.4

Re: [mdadm PATCH 1/1] Fix a build error

[Jes Sorensen]

On 09/19/2017 11:48 PM, Xiao Ni wrote:
> On the s390 platform the build fails with the error below.
> Manage.c: In function 'Manage_subdevs':
> Manage.c:1502:5: error: passing argument 3 of 'fstat_is_blkdev' from incompatible pointer type [-Werror]
>       fstat_is_blkdev(tfd, dv->devname, &rdev);
>       ^
> In file included from Manage.c:25:0:
> mdadm.h:1446:12: note: expected 'dev_t *' but argument is of type 'long unsigned int *'
> 
> It was introduced by commit 0a6bff09 (mdadm/util: unify fstat
> checking blkdev into function). It needs to pass a type 'dev_t'
> argument to fstat_is_blkdev, but it passes a type 'unsigned
> long' argument. So use a temporary variable to fix this.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> Suggested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Suggested-by: Wols Lists <antlists@youngman.org.uk>
> ---
>   Manage.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

So having a quick look at this, I have to say I don't like the casting 
back and forth. The fact that we carry rdev in an unsigned long in 
Manage_subdevs() seems dubious to me.

Did you look into what the implications would be to change it to a dev_t?

Jes

Re: [mdadm PATCH 1/1] Fix a build error

[Xiao Ni]

On 09/30/2017 05:47 AM, Jes Sorensen wrote:
> On 09/19/2017 11:48 PM, Xiao Ni wrote:
>> On the s390 platform the build fails with the error below.
>> Manage.c: In function 'Manage_subdevs':
>> Manage.c:1502:5: error: passing argument 3 of 'fstat_is_blkdev' from 
>> incompatible pointer type [-Werror]
>>       fstat_is_blkdev(tfd, dv->devname, &rdev);
>>       ^
>> In file included from Manage.c:25:0:
>> mdadm.h:1446:12: note: expected 'dev_t *' but argument is of type 
>> 'long unsigned int *'
>>
>> It was introduced by commit 0a6bff09 (mdadm/util: unify fstat
>> checking blkdev into function). It needs to pass a type 'dev_t'
>> argument to fstat_is_blkdev, but it passes a type 'unsigned
>> long' argument. So use a temporary variable to fix this.
>>
>> Signed-off-by: Xiao Ni <xni@redhat.com>
>> Suggested-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Suggested-by: Wols Lists <antlists@youngman.org.uk>
>> ---
>>   Manage.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> So having a quick look at this, I have to say I don't like the casting 
> back and forth. The fact that we carry rdev in an unsigned long in 
> Manage_subdevs() seems dubious to me.
>
> Did you look into what the implications would be to change it to a dev_t?
>
> Jes

Hi Jes

Do you mean define rdev as dev_t at first? It will change a lot if we do 
so. The argument rdev is passed
to many functions now. Such as Manage_add, Manage_remove, 
hot_remove_disk and so on. So I think
it's the reason why we carry rdev in an unsigned long in Manage_subddevs().

Do you have a better solution?

Best Regards
Xiao

Re: [mdadm PATCH 1/1] Fix a build error

[Jes Sorensen]

On 09/29/2017 09:19 PM, Xiao Ni wrote:
> 
> 
> On 09/30/2017 05:47 AM, Jes Sorensen wrote:
>> On 09/19/2017 11:48 PM, Xiao Ni wrote:
>>> On the s390 platform the build fails with the error below.
>>> Manage.c: In function 'Manage_subdevs':
>>> Manage.c:1502:5: error: passing argument 3 of 'fstat_is_blkdev' from 
>>> incompatible pointer type [-Werror]
>>>       fstat_is_blkdev(tfd, dv->devname, &rdev);
>>>       ^
>>> In file included from Manage.c:25:0:
>>> mdadm.h:1446:12: note: expected 'dev_t *' but argument is of type 
>>> 'long unsigned int *'
>>>
>>> It was introduced by commit 0a6bff09 (mdadm/util: unify fstat
>>> checking blkdev into function). It needs to pass a type 'dev_t'
>>> argument to fstat_is_blkdev, but it passes a type 'unsigned
>>> long' argument. So use a temporary variable to fix this.
>>>
>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>> Suggested-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>> Suggested-by: Wols Lists <antlists@youngman.org.uk>
>>> ---
>>>   Manage.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> So having a quick look at this, I have to say I don't like the casting 
>> back and forth. The fact that we carry rdev in an unsigned long in 
>> Manage_subdevs() seems dubious to me.
>>
>> Did you look into what the implications would be to change it to a dev_t?
>>
>> Jes
> 
> Hi Jes
> 
> Do you mean define rdev as dev_t at first? It will change a lot if we do 
> so. The argument rdev is passed
> to many functions now. Such as Manage_add, Manage_remove, 
> hot_remove_disk and so on. So I think
> it's the reason why we carry rdev in an unsigned long in Manage_subddevs().
> 
> Do you have a better solution?

I looked at the other functions and they all carry rdev as a dev_t so I 
made it the same in Manage_subdevs(). We may have to clean up some more 
functions to take the right input argument, but it looks like the 
correct solution to me.

Cheers,
Jes

Re: [mdadm PATCH 1/1] Fix a build error

[Xiao Ni]

On 09/30/2017 09:08 PM, Jes Sorensen wrote:
> On 09/29/2017 09:19 PM, Xiao Ni wrote:
>>
>>
>> On 09/30/2017 05:47 AM, Jes Sorensen wrote:
>>> On 09/19/2017 11:48 PM, Xiao Ni wrote:
>>>> On the s390 platform the build fails with the error below.
>>>> Manage.c: In function 'Manage_subdevs':
>>>> Manage.c:1502:5: error: passing argument 3 of 'fstat_is_blkdev' 
>>>> from incompatible pointer type [-Werror]
>>>>       fstat_is_blkdev(tfd, dv->devname, &rdev);
>>>>       ^
>>>> In file included from Manage.c:25:0:
>>>> mdadm.h:1446:12: note: expected 'dev_t *' but argument is of type 
>>>> 'long unsigned int *'
>>>>
>>>> It was introduced by commit 0a6bff09 (mdadm/util: unify fstat
>>>> checking blkdev into function). It needs to pass a type 'dev_t'
>>>> argument to fstat_is_blkdev, but it passes a type 'unsigned
>>>> long' argument. So use a temporary variable to fix this.
>>>>
>>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>>> Suggested-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>>> Suggested-by: Wols Lists <antlists@youngman.org.uk>
>>>> ---
>>>>   Manage.c | 6 ++++--
>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> So having a quick look at this, I have to say I don't like the 
>>> casting back and forth. The fact that we carry rdev in an unsigned 
>>> long in Manage_subdevs() seems dubious to me.
>>>
>>> Did you look into what the implications would be to change it to a 
>>> dev_t?
>>>
>>> Jes
>>
>> Hi Jes
>>
>> Do you mean define rdev as dev_t at first? It will change a lot if we 
>> do so. The argument rdev is passed
>> to many functions now. Such as Manage_add, Manage_remove, 
>> hot_remove_disk and so on. So I think
>> it's the reason why we carry rdev in an unsigned long in 
>> Manage_subddevs().
>>
>> Do you have a better solution?
>
> I looked at the other functions and they all carry rdev as a dev_t so 
> I made it the same in Manage_subdevs(). We may have to clean up some 
> more functions to take the right input argument, but it looks like the 
> correct solution to me.

Do you mean some changes like this:

diff --git a/Manage.c b/Manage.c
index 871d342..21536f5 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1367,7 +1367,7 @@ int Manage_subdevs(char *devname, int fd,
         }

         for (dv = devlist; dv; dv = dv->next) {
-               unsigned long rdev = 0; /* device to add/remove etc */
+               dev_t rdev = 0; /* device to add/remove etc */
                 int rv;
                 int mj,mn;

diff --git a/sysfs.c b/sysfs.c
index 78d2b52..dc44e38 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -78,7 +78,7 @@ int sysfs_open(char *devnm, char *devname, char *attr)
         return fd;
  }

-void sysfs_init_dev(struct mdinfo *mdi, unsigned long devid)
+void sysfs_init_dev(struct mdinfo *mdi, dev_t devid)
  {
         snprintf(mdi->sys_name,
                  sizeof(mdi->sys_name), "dev-%s", devid2kname(devid));


We carry rdev as a dev_t and then clean up other functions. If it's ok,
I'll send a new patch.

Best Regards
Xiao

Re: [mdadm PATCH 1/1] Fix a build error

[Jes Sorensen]

On 09/30/2017 10:12 AM, Xiao Ni wrote:
> 
> 
> On 09/30/2017 09:08 PM, Jes Sorensen wrote:
>> On 09/29/2017 09:19 PM, Xiao Ni wrote:
>>> Do you have a better solution?
>>
>> I looked at the other functions and they all carry rdev as a dev_t so 
>> I made it the same in Manage_subdevs(). We may have to clean up some 
>> more functions to take the right input argument, but it looks like the 
>> correct solution to me.
> 
> Do you mean some changes like this:
> 
> diff --git a/Manage.c b/Manage.c
> index 871d342..21536f5 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -1367,7 +1367,7 @@ int Manage_subdevs(char *devname, int fd,
>          }
> 
>          for (dv = devlist; dv; dv = dv->next) {
> -               unsigned long rdev = 0; /* device to add/remove etc */
> +               dev_t rdev = 0; /* device to add/remove etc */
>                  int rv;
>                  int mj,mn;
> 
> diff --git a/sysfs.c b/sysfs.c
> index 78d2b52..dc44e38 100644
> --- a/sysfs.c
> +++ b/sysfs.c
> @@ -78,7 +78,7 @@ int sysfs_open(char *devnm, char *devname, char *attr)
>          return fd;
>   }
> 
> -void sysfs_init_dev(struct mdinfo *mdi, unsigned long devid)
> +void sysfs_init_dev(struct mdinfo *mdi, dev_t devid)
>   {
>          snprintf(mdi->sys_name,
>                   sizeof(mdi->sys_name), "dev-%s", devid2kname(devid));
> 
> 
> We carry rdev as a dev_t and then clean up other functions. If it's ok,
> I'll send a new patch.
> 
> Best Regards
> Xiao

Yes,

But check latest git first, I already pushed in some of the changes.

Cheers,
Jes

Re: [mdadm PATCH 1/1] Fix a build error

[Xiao Ni]

On 10/02/2017 06:08 AM, Jes Sorensen wrote:
> On 09/30/2017 10:12 AM, Xiao Ni wrote:
>>
>>
>> On 09/30/2017 09:08 PM, Jes Sorensen wrote:
>>> On 09/29/2017 09:19 PM, Xiao Ni wrote:
>>>> Do you have a better solution?
>>>
>>> I looked at the other functions and they all carry rdev as a dev_t 
>>> so I made it the same in Manage_subdevs(). We may have to clean up 
>>> some more functions to take the right input argument, but it looks 
>>> like the correct solution to me.
>>
>> Do you mean some changes like this:
>>
>> diff --git a/Manage.c b/Manage.c
>> index 871d342..21536f5 100644
>> --- a/Manage.c
>> +++ b/Manage.c
>> @@ -1367,7 +1367,7 @@ int Manage_subdevs(char *devname, int fd,
>>          }
>>
>>          for (dv = devlist; dv; dv = dv->next) {
>> -               unsigned long rdev = 0; /* device to add/remove etc */
>> +               dev_t rdev = 0; /* device to add/remove etc */
>>                  int rv;
>>                  int mj,mn;
>>
>> diff --git a/sysfs.c b/sysfs.c
>> index 78d2b52..dc44e38 100644
>> --- a/sysfs.c
>> +++ b/sysfs.c
>> @@ -78,7 +78,7 @@ int sysfs_open(char *devnm, char *devname, char *attr)
>>          return fd;
>>   }
>>
>> -void sysfs_init_dev(struct mdinfo *mdi, unsigned long devid)
>> +void sysfs_init_dev(struct mdinfo *mdi, dev_t devid)
>>   {
>>          snprintf(mdi->sys_name,
>>                   sizeof(mdi->sys_name), "dev-%s", devid2kname(devid));
>>
>>
>> We carry rdev as a dev_t and then clean up other functions. If it's ok,
>> I'll send a new patch.
>>
>> Best Regards
>> Xiao
>
> Yes,
>
> But check latest git first, I already pushed in some of the changes.
>
> Cheers,
> Jes
>

Hi Jes

I checked the patches you pushed recent. I backport them and the problem 
can be fixed already.
Thanks for the help.

Best Regards
Xiao