D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[Kevin Funk]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  This definitely needs another revision. Please fix the outstanding issues.

INLINE COMMENTS

> iokitblock.h:43
> +
> +    virtual int deviceMajor() const;
> +    virtual int deviceMinor() const;

Here and below: Missing `Q_DECL_OVERRIDE`

> iokitdevice.cpp:151
> +    size_t size = 0;
> +    if (sysctlbyname("hw.model", NULL, &size, NULL, 0) == 0 && size > 0) {
> +        model = new char [size];

Use `nullptr` everywhere

> iokitdevice.cpp:154
> +        if (sysctlbyname("hw.model", model, &size, NULL, 0) == 0) {
> +            qModel= QLatin1String(model);
> +        }

Style: Use `qModel = ...`

> iokitdevice.cpp:156
> +        }
> +        delete model;
> +    }

`new`/`delete` mismatch. Use `delete[]`

> iokitdevice.cpp:167
> +    {
> +        type << Solid::DeviceInterface::Unknown;
> +    }

Initialize at class member decl

> iokitdevice.cpp:171
> +    {
> +        if (parent) {
> +            delete parent;

That's *very* odd style. Why does a private class delete its public counterpart? I've never seen this.

> iokitdevice.cpp:197
> +    type = typesFromEntry(entry, properties, mainType);
> +//     if (udi.contains(QStringLiteral("IOBD")) || udi.contains(QStringLiteral("BD PX"))) {
> +//         qWarning() << "Solid: BlueRay entry" << entry << "mainType=" << mainType << "typeList:" << type

Don't leave commented code around. Either enable this code paths properly via categorized logging or remove it altogether.

> iokitopticaldrive.cpp:211
> +    }
> +    delete ioDVDServices;
> +    d = new Private(device, devCharMap);

Create on `ioDVDServices` on the stack to begin with?

> iokitopticaldrive.cpp:333
> +{
> +    QList<int> speeds;
> +    return speeds;

`return {}`

> iokitopticaldrive.cpp:354
> +        return true;
> +    } else {
> +        emit ejectDone(Solid::ErrorType::OperationFailed, QVariant(), m_device->udi());

Coding style: Would turn around this two branches (and remove the else part) to make the intended meaning more clear:

Pseudo code:

  if (errorCase) {
    return ...;
  }
  
  // normal case, code flow continues
  return ...;

That's usually the common style

> iokitprocessor.cpp:84
> +
> +const QString Processor::vendor()
> +{

Remove `const`

> iokitprocessor.cpp:87
> +    QString qVendor = QString();
> +    char *vendor = NULL;
> +    size_t size = 0;

Way too much error prone `new`/`delete` logic on naked char arrays. Every invocation here is wrong (causing undefined behavior) due to the `new[]`/delete` mismatch mentioned above.

Factor that out reading into a `QString` into a separate function, cf. https://github.com/trueos/lumina/blob/master/src-qt5/core/libLumina/LuminaOS-DragonFly.cpp#L29 and use it everywhere instead.

> iokitprocessor.cpp:99
>  
> +const QString Processor::product()
> +{

Remove `const`

> iokitstorage.cpp:36
> +    {
> +        daRef = 0;
> +        daDict = 0;

Here and one line below: Could be initialized at decl again.

> iokitstorage.cpp:36
> +    {
> +        daRef = 0;
> +        daDict = 0;

`nullptr`

> iokitstorage.cpp:51
> +            CFRelease(daDict);
> +            daDict = 0;
> +        }

`nullptr`, etc. pp.

> iokitstorage.cpp:75
> +
> +IOKitStorage::IOKitStorage(const IOKitDevice *device)
> +    : Block(device)

Please just remove the constructors taking a `const IOKitDevice *device` and adapt uses (just use `IOKitDevice *device` everywhere). It's unusual for pointer types to be `const` in such contexts. It just adds noise.

> iokitstorageaccess.cpp:41
> +        } else {
> +            daRef = 0;
> +        }

Here and below: `nullptr`

> iokitstorageaccess.cpp:91
> +{
> +    // mount points can change (but can they between invocations of filePath()?)
> +    if (d->daRef) {

Early-return would reduce the indentation level here [1].

  if (errorCase)
    // return early
    return {};
  }
  
  // normal case
  // ...

[1] https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

> iokitstorageaccess.cpp:132
> +        return false;
> +    } else {
> +        // TODO?

Coding style: Eliminate `else` branch, just `return in function-level scope.

> iokitstorageaccess.cpp:142
> +        return false;
> +    } else {
> +        // TODO?

Dito

> iokitvolume.cpp:42
> +        } else {
> +            daRef = 0;
> +        }

Here and below: `nullptr`

> iokitvolume.cpp:121
> +        if (dict) {
> +//             qWarning() << Q_FUNC_INFO;
> +//             CFShow(dict);

Remove commented code.

> iokitvolume.cpp:177
> +    if (d->daRef) {
> +        CFDictionaryRef dict = DADiskCopyDescription(d->daRef);
> +        if (dict) {

That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, `product, `description`. I'm sure that can be done better.

> iokitvolume.h:45
> +
> +    virtual bool isIgnored() const;
> +    virtual Solid::StorageVolume::UsageType usage() const;

Here and below: Missing `Q_DECL_OVERRIDE` again

> iokitvolume.h:53
> +
> +    const QString vendor();
> +    const QString product();

Here and below: No `const` for return types on implicitly-shared types (just adds noise)

The functions can be made `const` though.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20171023/13c49a2d/attachment-0001.html>