On Fri Jun 07, 2024 at 12:20:41PM +0200, Omar Polo wrote:
> On 2024/06/07 11:20:29 +0200, Rafael Sadowski <rafael@sizeofvoid.org> wrote:
> > Please find below a simple patch to add control battery charging support
> > in powerdevil aka KDE Plamsa - systemsettings via sysctl(2).
> > 
> > I would be happy if someone would take a look at the code.
> 
> never used plasma so haven't tested, but fwiw the sysctl(2) bits looks
> fine to me.

Thanks for your feedback!

> 
> > [...]
> > ++#if defined(__OpenBSD__)
> > ++static bool isThresholdSupported()
> > ++{
> > ++    int mode;
> > ++    size_t len = sizeof(mode);
> > ++    int mib[] = {CTL_HW, HW_BATTERY, HW_BATTERY_CHARGEMODE};
> > ++    return (sysctl(mib, 3, &mode, &len, NULL, 0) != -1);
> 
> I was a bit surprised by this initially.  However, you're just testing
> whether the hw.battery node is available, so this seems fine to me.
> Maybe add a comment just in case?  Just to save a few seconds to the
> next one glancing at this.

Yes, that was my idea, I have no other way of checking whether this feature is
available. Good point I'll add a comment.

> 
> > ++}
> > ++
> > ++static int getBatteryCharge(const int type)
> > ++{
> > ++    int percentage = -1;
> > ++    size_t len = sizeof(percentage);
> > ++    int mib[] = {CTL_HW, HW_BATTERY, type};
> > ++    sysctl(mib, 3, &percentage, &len, NULL, 0);
> 
> maybe consider checking whether sysctl returns successfully here or not.
> If we call this, it means that thet hw.battery node exists, so this
> shouldn't be failing, but still it doesn't cost much to add an if.

Yeah, I didn't see the point. if sysctl(2) fails I return -1 (default).  If it
passed I return the value set by sysctl. I safe code and one more return
statement.
> 
> > ++    return percentage;
> > ++}
> 
> 
> 
> Thanks,
> 
> Omar Polo
>