Is that RFC to be part of a series (as the subject "2/2" suggests)?

On Mon, Apr 20, 2015 at 07:28:23AM +0000, 'BSRK Aditya' via ganeti-devel wrote:
> These functions now make RPCs to WConfd
> instead of locking the config.

This comment is a bit misleading, as WConfd will lock the config
on behalf of the caller.

Also, the log message is not a complete description of what is happening
in this patch.

> -      
> +

Unrelated whitespace change?

> +      let check = checkUUIDpresent cs disk
> +      unless check . Bad . ConfigurationError $ printf
> +             "Cannot add %s: UUID %s already in use"
> +             (show $ diskName disk) (diskUuid disk)

as "check" is used only once and the definition is short, why not inline?

> +    else do
> +      let check = checkUniqueUUID cs disk
> +      unless check . Bad . ConfigurationError $ printf
> +             "Cannot replace %s: UUID %s not present"
> +             (show $ diskName disk) (diskUuid disk)

dito

> +  when (idx < 0) . Bad . GenericError $
> +    "Not accepting negative indices other than -1"

The test and the error message do not fit together.


The rest looks good.

-- 
Klaus Aehlig
Google Germany GmbH, Dienerstr. 12, 80331 Muenchen
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores

Reply via email to