On Tue, Sep 03, 2019 at 10:31:25PM +0200, Heinrich Schuchardt wrote: > On 9/3/19 7:40 AM, AKASHI Takahiro wrote: > >If EFI_VARIABLE_APPEND_WRITE is specified in attributes at > >efi_set_variable(), specified data will be appended to the variable's > >original value. Attributes other than APPEND_WRITE should not be > >modified. > > > >With this patch, APPEND_WRITE test in 'variables' selftest will pass. > > > >Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> > >--- > > lib/efi_loader/efi_variable.c | 50 +++++++++++++++++++++-------------- > > 1 file changed, 30 insertions(+), 20 deletions(-) > > > >diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > >index 6687b69a400d..5d1ee50a606e 100644 > >--- a/lib/efi_loader/efi_variable.c > >+++ b/lib/efi_loader/efi_variable.c > >@@ -423,18 +423,17 @@ efi_status_t EFIAPI efi_set_variable(u16 > >*variable_name, > > const efi_guid_t *vendor, u32 attributes, > > efi_uintn_t data_size, const void *data) > > { > >- char *native_name = NULL, *val = NULL, *s; > >+ char *native_name = NULL, *oval, *val = NULL, *s; > > Please, use old_val install of oval.
Hmm, okay. > >+ size_t oval_size; > > Please, use old_val_size. > > > efi_status_t ret = EFI_SUCCESS; > > u32 attr; > > > > EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes, > > data_size, data); > > > >- /* TODO: implement APPEND_WRITE */ > > if (!variable_name || !*variable_name || !vendor || > > ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) && > >- !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) || > >- (attributes & EFI_VARIABLE_APPEND_WRITE)) { > >+ !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) { > > ret = EFI_INVALID_PARAMETER; > > goto out; > > } > > Append with data_size = 0 should not delete the variable. See UEFI 2.8 spec. Good point. Will add a check against it. > If a variable does not exist, trying to delete it should return > EFI_NOT_FOUND. > > If a variable is READ_ONLY we have to return EFI_WRITE_PROTECTED and > should not delete it. > > I know these deficiencies are not caused by your patch. But as you are > touching the code now, could you, please, consider them. Okay. > >@@ -452,28 +451,37 @@ efi_status_t EFIAPI efi_set_variable(u16 > >*variable_name, > > goto out; > > } > > > >- val = env_get(native_name); > >- if (val) { > >- parse_attr(val, &attr); > >+ oval = env_get(native_name); > >+ if (oval) { > >+ oval = parse_attr(oval, &attr); > > > >- /* We should not free val */ > >- val = NULL; > > if (attr & READ_ONLY) { > > > ret = EFI_WRITE_PROTECTED; > > goto out; > > } > > > >- /* > >- * attributes won't be changed > >- * TODO: take care of APPEND_WRITE once supported > >- */ > >- if (attr != attributes) { > >+ /* attributes won't be changed */ > >+ if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) { > > ret = EFI_INVALID_PARAMETER; > > goto out; > > } > >+ > >+ if (attributes & EFI_VARIABLE_APPEND_WRITE) { > >+ if (!prefix(oval, "(blob)")) { > >+ /* TODO: should support utf8? */ > > If the variable is read_only we should not append to it but return > EFI_WRITE_PROTECTED. > > We never stored any data as utf8. We can eliminate all references to the > (utf8) prefix in a future patch. Agree here. > In lib/efi_selftest/efi_selftest_variables.c we already have a test for > APPEND. We should change the efi_st_todo() into efi_st_error() in a > follow-up patch. Okay. I know that my patch passes the test itself. -Takahiro Akashi > Thanks for taking care of this gap. > > Best regards > > Heinrich > > >+ return EFI_DEVICE_ERROR; > >+ goto out; > >+ } > >+ oval_size = strlen(oval); > >+ } else { > >+ oval_size = 0; > >+ } > >+ } else { > >+ oval_size = 0; > > } > > > >- val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1); > >+ val = malloc(oval_size + 2 * data_size > >+ + strlen("{ro,run,boot,nv}(blob)") + 1); > > if (!val) { > > ret = EFI_OUT_OF_RESOURCES; > > goto out; > >@@ -481,10 +489,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, > > > > s = val; > > > >- /* > >- * store attributes > >- * TODO: several attributes are not supported > >- */ > >+ /* store attributes */ > > attributes &= (EFI_VARIABLE_NON_VOLATILE | > > EFI_VARIABLE_BOOTSERVICE_ACCESS | > > EFI_VARIABLE_RUNTIME_ACCESS); > >@@ -505,8 +510,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, > > } > > s += sprintf(s, "}"); > > > >+ if (oval_size) > >+ /* APPEND_WRITE */ > >+ s += sprintf(s, oval); > >+ else > >+ s += sprintf(s, "(blob)"); > >+ > > /* store payload: */ > >- s += sprintf(s, "(blob)"); > > s = bin2hex(s, data, data_size); > > *s = '\0'; > > > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot