Re: [PATCH] efivars update

[Matt Domsch]

> The patch is against 2.6.0-test4.  Please apply. 

Thanks for your work on this!  One bug, 3 nits...

> +static ssize_t
> +efivar_edit(struct efivar_entry *entry, const char *buf, size_t count)
...
> +       spin_lock(&efivars_lock);
> +       status = efi.set_variable(new_var->VariableName,
> +                                 &new_var->VendorGuid,
> +                                 new_var->Attributes,
> +                                 new_var->DataSize,
> +                                 new_var->Data);
> +
> +       if (status != EFI_SUCCESS) {
> +               printk(KERN_WARNING "set_variable() failed: status=%lx\n",
> +                               status);
> +               return -EIO;
> +       }
> +       spin_unlock(&efivars_lock);


Move the unlock up above the status test, else you return while
holding the lock on failure.


> +static ssize_t
> +efivar_create(struct subsystem *sub, const char *buf, size_t count)
...
> +       if (found) {
> +               printk("EFI variable already exists!\n");

level on printk please.



> +efivar_delete(struct subsystem *sub, const char *buf, size_t count)
...
> +       status = efi.set_variable(del_var->VariableName,
> +                                 &del_var->VendorGuid,
> +                                 del_var->Attributes,
> +                                 del_var->DataSize,
> +                                 del_var->Data);

Might as well just force the deletion by setting Attributes=0 or
DataSize=0 or both.  Less chance for userspace error.


> + * Overrides for Emacs so that we follow Linus's tabbing style.

Please kill this chunk.  I shouldn't have had it there in the first 
place. :-)

Looks great to me!  Thanks again.


-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/