Re: [Bug-ddrescue] [PATCH] Add device_id() for Cygwin

[Christian Franke]

Hello Antonio,

Antonio Diaz Diaz wrote:
> Christian Franke wrote:
> > the attached patch adds device_id() for Cygwin. Tested with 32- and 
> > 64-bit Cygwin.
> > Should be safe to include in 1.21 as no other platform is affected.
>
> Thanks for the patch.
>
> The patch is mostly ok but, isn't there a safer way of implementing 
> this for Cygwin? I don't like neither the use of an union nor the 
> unverified access to the 'raw' member in the union 
> ('data.raw[data.desc.VendorIdOffset]', for example). Most probably 
> this will cause problems with '-fstrict-aliasing', or even an invalid 
> memory access.

New attached new patch avoids the union, does strict index checks and 
ensures zero termination. It now also allows that an ATA IDENTIFY string 
is only in VendorId or ProductId which occasionally happened with some 
older drives.

Tested with g++ 4.9.3 and clang++ 3.5.2.

BTW: I guess both approaches (char buffer + cast to *struct, union of 
buf + struct) are safe:
"A character type may alias any other type."
"Even with -fstrict-aliasing, type-punning is allowed, provided the 
memory is accessed through the union type."
https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/Optimize-Options.html#index-fstrict-aliasing-926

Best regards,
Christian

ddrescue-cygwin-device_id-3.patch.txt
Description: Text document