RFC: registry keys of serial ports

[Stefan Leichter]

Hi,

i like to know what modifikations the attached patch needs to go into git.

More things need to be done in this area. I know of:
- more registry keys [1]
- remove the registry keys when a serial port disappears (USB)
- the links in the dosdevice directory needs to be created

Patches for the items above will be created, when the current patch went into 
git

References:
1 http://www.winehq.org/pipermail/wine-patches/2009-April/071999.html
- http://www.winehq.org/pipermail/wine-devel/2009-April/074476.html

registry keys of serial ports

[Dmitry Timoshkov]

"Stefan Leichter"  wrote:
> i like to know what modifikations the attached patch needs to go into git.

1. patch needs to be created with git
2. there is no need to create a new file
> @@ -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?
> -
>
> +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'.
> -
> - 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.
> - WCHAR data[10];
> -
> - attr.Length = sizeof(attr);
> - attr.RootDirectory = 0;
> - attr.ObjectName = (UNICODE_STRING*) &serialcom_str;

You should not cast away 'const'.

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"  wrote:
> > i like to know what modifikations the attached patch needs to go into
> > git.
> - patch needs to be created with git

Next time you will tell me what editor to use ;-). This was _never_ needed 
before
> - 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.

[Christian Gmeiner]

Hi Stefan,

I like the idea of adding more registry keys for hardware components.
I want some registry keys for usb devices
so I had a look at your patch. I don't like the mess with the added
goto serial. If in future there will be support
for usb and the whole source will be quite hard to follow. The usb
registry keys are needed e.g. for this [1].
So i like to propose my idea how it could look like.

In function new_device( LibHalContext *ctx, const char *udi ) we could
readout the info.subsystem setting of the newly added device and add a
if for info.subsystem strings like
block - block devices
tty - serial
usb_devices/usb - usb

could look like

/* HAL callback for new device */
static void new_device( LibHalContext *ctx, const char *udi )
{
    DBusError error;
    char *subsystem = NULL;

p_dbus_error_init( &error );

if (!(subsystem = p_libhal_device_get_property_string( ctx, udi,
"info.subsystem", &error )))
        goto done;

if (strcmp(subsystem == "block") == 0)
        block_device( ctx, udi, &error );
    else if (strcmp(subsystem == "usb_device") == 0)
        usb_device( ctx, udi, &error );
    else if (strcmp(subsystem == "tty") == 0)
        serial_device( ctx, udi, &error );

p_dbus_error_free( &error );
}

Now we have a ability to handle each device type with an own function, like

static void block_device( LibHalContext *ctx, const char *udi,
DBusError *error )
{
    char *device = NULL;
    char *mount_point = NULL;
    char *parent = NULL;
    char *uuid_str = NULL;
    char *type = NULL;
    GUID guid, *guid_ptr = NULL;
    enum device_type drive_type;

if (!(device = p_libhal_device_get_property_string( ctx, udi,
"block.device", error )))
        goto done;

if (!(mount_point = p_libhal_device_get_property_string( ctx, udi,
"volume.mount_point", error )))
        goto done;

if (!(parent = p_libhal_device_get_property_string( ctx, udi,
"info.parent", error )))
        goto done;

if (!(uuid_str = p_libhal_device_get_property_string( ctx, udi,
"volume.uuid", error )))
        p_dbus_error_free( &error );  /* ignore error */
    else
        guid_ptr = parse_uuid( &guid, uuid_str );

if (!(type = p_libhal_device_get_property_string( ctx, parent,
"storage.drive_type", error )))
        p_dbus_error_free( &error );  /* ignore error */

if (type && !strcmp( type, "cdrom" )) drive_type = DEVICE_CDROM;
    else if (type && !strcmp( type, "floppy" )) drive_type = DEVICE_FLOPPY;
    else drive_type = DEVICE_UNKNOWN;

if (p_libhal_device_get_property_bool( ctx, parent,
"storage.removable", error ))
    {
        add_dos_device( -1, udi, device, mount_point, drive_type, guid_ptr );
        /* add property watch for mount point */
        p_libhal_device_add_property_watch( ctx, udi, error );
    }
    else if (guid_ptr) add_volume( udi, device, mount_point,
DEVICE_HARDDISK_VOL, guid_ptr );

done:
    if (type) p_libhal_free_string( type );
    if (parent) p_libhal_free_string( parent );
    if (device) p_libhal_free_string( device );
    if (uuid_str) p_libhal_free_string( uuid_str );
    if (mount_point) p_libhal_free_string( mount_point );
}

Keep in mind that i am not sure if this source samples compile - was a
quick copy & paste action.

What do you think about it?

[1] http://wiki.winehq.org/USB

[Stefan Leichter]

Am Friday 11 September 2009 12:30:01 schrieb Christian Gmeiner:
> Hi Stefan,
>
> I like the idea of adding more registry keys for hardware components.
> I want some registry keys for usb devices
> so I had a look at your patch. I don't like the mess with the added
> goto serial. If in future there will be support
> for usb and the whole source will be quite hard to follow. The usb
> registry keys are needed e.g. for this [1].
> So i like to propose my idea how it could look like.
>
> In function new_device( LibHalContext *ctx, const char *udi ) we could
> readout the info.subsystem setting of the newly added device and add a
> if for info.subsystem strings like
> block - block devices
> tty - serial
> usb_devices/usb - usb
>
> could look like
>
> /* HAL callback for new device */
> static void new_device( LibHalContext *ctx, const char *udi )
> {
>     DBusError error;
>     char *subsystem = NULL;
> p_dbus_error_init( &error );
>
> if (!(subsystem = p_libhal_device_get_property_string( ctx, udi,
>
> "info.subsystem", &error )))
>         goto done;
> if (strcmp(subsystem == "block") == 0)
>     block_device( ctx, udi, &error );
> else if (strcmp(subsystem == "usb_device") == 0)
>     usb_device( ctx, udi, &error );
> else if (strcmp(subsystem == "tty") == 0)
>     serial_device( ctx, udi, &error );
>
> p_dbus_error_free( &error );
>
> }

Hi Christian,

i like your idea, but a short test shows that "info.subsystem" is not 
available for drives. But "info.category" can be used.

[Christian Gmeiner]

2009/9/11 Stefan Leichter :
> Am Friday 11 September 2009 12:30:01 schrieb Christian Gmeiner:
> > Hi Stefan,
> >
> > I like the idea of adding more registry keys for hardware components.
> > I want some registry keys for usb devices
> > so I had a look at your patch. I don't like the mess with the added
> > goto serial. If in future there will be support
> > for usb and the whole source will be quite hard to follow. The usb
> > registry keys are needed e.g. for this [1].
> > So i like to propose my idea how it could look like.
> >
> > In function new_device( LibHalContext *ctx, const char *udi ) we could
> > readout the info.subsystem setting of the newly added device and add a
> > if for info.subsystem strings like
> > block - block devices
> > tty - serial
> > usb_devices/usb - usb
> >
> > could look like
> >
> > /* HAL callback for new device */
> > static void new_device( LibHalContext *ctx, const char *udi )
> > {
> >     DBusError error;
> >     char *subsystem = NULL;
> >
> > p_dbus_error_init( &error );
> >
> > if (!(subsystem = p_libhal_device_get_property_string( ctx, udi,
> > "info.subsystem", &error )))
> >         goto done;
> >
> > if (strcmp(subsystem == "block") == 0)
> >         block_device( ctx, udi, &error );
> >     else if (strcmp(subsystem == "usb_device") == 0)
> >         usb_device( ctx, udi, &error );
> >     else if (strcmp(subsystem == "tty") == 0)
> >         serial_device( ctx, udi, &error );
> >
> > p_dbus_error_free( &error );
> > }
>
> Hi Christian,
>
> i like your idea, but a short test shows that "info.subsystem" is not
> available for drives. But "info.category" can be used.

with drives you mean CD and DVD drives?

lshal gives me this:

udi = '/org/freedesktop/Hal/devices/pci_10de_37f_1_scsi_host_scsi_device_lun0'
  info.linux.driver = 'sr'  (string)
  info.parent =
'/org/freedesktop/Hal/devices/pci_10de_37f_1_scsi_host'  (string)
  info.product = 'SCSI Device'  (string)
  info.subsystem = 'scsi'  (string)
  info.udi = '/org/freedesktop/Hal/devices/pci_10de_37f_1_scsi_host_scsi_device_lun0'
 (string)
  linux.hotplug_type = 2  (0x2)  (int)
  linux.subsystem = 'scsi'  (string)
  linux.sysfs_path =
'/sys/devices/pci0000:00/0000:00:0d.2/host4/target4:0:0/4:0:0:0'
(string)
  scsi.bus = 0  (0x0)  (int)
  scsi.host = 4  (0x4)  (int)
  scsi.lun = 0  (0x0)  (int)
  scsi.model = 'CD/DVDW SH-S183L'  (string)
  scsi.target = 0  (0x0)  (int)
  scsi.type = 'cdrom'  (string)
  scsi.vendor = 'TSSTcorp'  (string)

udi = '/org/freedesktop/Hal/devices/storage_model_CD_DVDW_SH_S183L'
  block.device = '/dev/sr0'  (string)
  block.is_volume = false  (bool)
  block.major = 11  (0xb)  (int)
  block.minor = 0  (0x0)  (int)
  block.storage_device =
'/org/freedesktop/Hal/devices/storage_model_CD_DVDW_SH_S183L'
(string)
  info.addons = {'hald-addon-storage'} (string list)
  info.capabilities = {'storage', 'block', 'storage.cdrom'} (string list)
  info.category = 'storage'  (string)
  info.interfaces = {'org.freedesktop.Hal.Device.Storage',
'org.freedesktop.Hal.Device.Storage',
'org.freedesktop.Hal.Device.Storage.Removable'} (string list)
  info.parent =
'/org/freedesktop/Hal/devices/pci_10de_37f_1_scsi_host_scsi_device_lun0'
 (string)
  info.product = 'CD/DVDW SH-S183L'  (string)
  info.udi = '/org/freedesktop/Hal/devices/storage_model_CD_DVDW_SH_S183L'
 (string)
  info.vendor = 'TSSTcorp'  (string)
  linux.hotplug_type = 3  (0x3)  (int)
  linux.sysfs_path =
'/sys/devices/pci0000:00/0000:00:0d.2/host4/target4:0:0/4:0:0:0/block/sr0'
 (string)
  org.freedesktop.Hal.Device.Storage.method_argnames =
{'extra_options', 'extra_options'} (string list)
  org.freedesktop.Hal.Device.Storage.method_execpaths =
{'hal-storage-eject', 'hal-storage-closetray'} (string list)
  org.freedesktop.Hal.Device.Storage.method_names = {'Eject',
'CloseTray'} (string list)
  org.freedesktop.Hal.Device.Storage.method_signatures = {'as', 'as'}
(string list)
  storage.automount_enabled_hint = true  (bool)
  storage.bus = 'pci'  (string)
  storage.cdrom.bd = false  (bool)
  storage.cdrom.bdr = false  (bool)
  storage.cdrom.bdre = false  (bool)
  storage.cdrom.cdr = true  (bool)
  storage.cdrom.cdrw = true  (bool)
  storage.cdrom.dvd = true  (bool)
  storage.cdrom.dvdplusr = true  (bool)
  storage.cdrom.dvdplusrdl = true  (bool)
  storage.cdrom.dvdplusrw = true  (bool)
  storage.cdrom.dvdplusrwdl = false  (bool)
  storage.cdrom.dvdr = true  (bool)
  storage.cdrom.dvdram = true  (bool)
  storage.cdrom.dvdrdl = true  (bool)
  storage.cdrom.dvdrw = true  (bool)
  storage.cdrom.hddvd = false  (bool)
  storage.cdrom.hddvdr = false  (bool)
  storage.cdrom.hddvdrw = false  (bool)
  storage.cdrom.mo = false  (bool)
  storage.cdrom.mrw = true  (bool)
  storage.cdrom.mrw_w = true  (bool)
  storage.cdrom.read_speed = 16620  (0x40ec)  (int)
  storage.cdrom.support_media_changed = true  (bool)
  storage.cdrom.support_multisession = true  (bool)
  storage.cdrom.write_speed = 11080  (0x2b48)  (int)
  storage.cdrom.write_speeds = {'11080', '5540'} (string list)
  storage.drive_type = 'cdrom'  (string)
  storage.firmware_version = 'SB01'  (string)
  storage.hotpluggable = false  (bool)
  storage.lun = 0  (0x0)  (int)
  storage.media_check_enabled = true  (bool)
  storage.model = 'CD/DVDW SH-S183L'  (string)
  storage.no_partitions_hint = true  (bool)
  storage.originating_device = '/org/freedesktop/Hal/devices/computer'  (string)
  storage.removable = true  (bool)
  storage.removable.media_available = true  (bool)
  storage.removable.media_size = 2717777920  (0xa1fe0000)  (uint64)
  storage.removable.support_async_notification = false  (bool)
  storage.requires_eject = true  (bool)
  storage.size = 0  (0x0)  (uint64)
  storage.vendor = 'TSSTcorp'  (string)

So it would be info.subsystem = 'scsi'