Re: registry keys of serial ports

[Stefan Leichter]

Hello Dmitry,

Thank You for your review, but

Am Friday 11 September 2009 10:31:14 schrieb Dmitry Timoshkov:
> "Stefan Leichter" <Stefan.Leichter(a)camline.com> wrote:
> > i like to know what modifikations the attached patch needs to go into
> > git.
>
> 1. patch needs to be created with git
> Next time you will tell me what editor to use ;-). This was _never_ needed 
> before
> 2. there is no need to create a new file
> Currently the new file is small, but it will grow in the future as i wrote. 
> Also the drive stuff has its own file.

I like to hear AJ opinion here.
> > @@ -175,6 +180,16 @@
> >          p_libhal_device_add_property_watch( ctx, udi, &error );
> >      }
> >      else if (guid_ptr) add_volume( udi, device, mount_point,
> > DEVICE_HARDDISK_VOL, guid_ptr ); +    goto done;
>
> Is there any reason you are doing 'goto done' above?

Technically you are right. But the goto will make clear the code related to 
the drive stuff is finished.
> > +
> > +serial:
> > +    if (!(device = p_libhal_device_get_property_string( ctx, udi,
> > "serial.device", &error ))) +        goto done;
> > +
> > +    if ( -1 == (port = p_libhal_device_get_property_int( ctx, udi,
> > "serial.port", &error ))) +        goto done;
>
> 'if ( -1 == port...' doesn't match existing style, 'if (port... == -1)'
> does. Same for other places.
> > +
> > +    add_serial_device(port, device);
> >
> > done:
> >      if (type) p_libhal_free_string( type );
> >
> > +NTSTATUS add_serial_device(int port, const char *device)
> > +{
> > +    static const WCHAR format_data[]  = {'C','O','M','%','d',0};
> > +    static const WCHAR serialcom[] = {'M','a','c','h','i','n','e','\\',
> > +                                     
> > 'H','A','R','D','W','A','R','E','\\', +                                  
> >    'D','E','V','I','C','E','M','A','P','\\', +                           
> >           'S','E','R','I','A','L','C','O','M','M',0}; +    static const
> > UNICODE_STRING serialcom_str
> > +                        = {sizeof(serialcom)-1, sizeof(serialcom),
> > (WCHAR*) serialcom};
>
> You should not cast away 'const'.
> What do you suggest to get around the compiler warning 
> "warning: assignment discards qualifiers from pointer target type"
> > +
> > +    CHAR value[50];
> > +    HANDLE targetKey = NULL;
> > +    NTSTATUS retval;
> > +    OBJECT_ATTRIBUTES attr;
> > +    UNICODE_STRING valueU = {0, 0, NULL};
>
> Initialization is not needed above since you are doing it anyway.
> Do you know what RtlFreeUnicodeString() does on uninitialized variables (on 
> all supported operating systems)? I will move some code to make sure the sure 
> the variable is initialize always
> > +    WCHAR data[10];
> > +
> > +    attr.Length = sizeof(attr);
> > +    attr.RootDirectory = 0;
> > +    attr.ObjectName = (UNICODE_STRING*) &serialcom_str;
>
> You should not cast away 'const'.

s.a.

-- 
Stefan